-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Replace TransportVersion.onOrAfter with TransportVersion.supports
#136760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple suggestions for followups
|
|
||
| @Override | ||
| public boolean before(TransportVersion version) { | ||
| return version.id() > id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we throw UOE here as well, suggesting .supports(...) == false? Or is that intended for a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will 100% be a follow up. I just prefer to do one method at at time given how large this change is.
| } | ||
|
|
||
| @Override | ||
| public boolean between(TransportVersion lowerInclusive, TransportVersion upperExclusive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the above, can this be blocked in a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
| * <li>{@code 8_801_0_00.isPatchFrom(8_800_0_04)}: {@code false}</li> | ||
| * </ul> | ||
| */ | ||
| public boolean isPatchFrom(TransportVersion version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are only a handful of remaining uses, perhaps these versions could be manually migrated (building a chained TransportVersion in memory) so that they can use supports and this could be made private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I will look into that.
|
I'm closing this in favor of other refactoring work first. |
Note this changes
TransportVersion.onOrAfterto throw anUnsupportedOperationException.ES-12334