Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ESQL: Introduce language versioning to REST API #106824
ESQL: Introduce language versioning to REST API #106824
Changes from 13 commits
1f45d65
c4d5a8e
26b9fe6
887759a
da6c0e1
8e6de63
98b816f
d5bef79
f0a2aae
9e97e43
11ba353
a29fc71
85576a7
5920994
a633632
c8c9684
de6304e
be3f255
5b15c30
c6f06db
eb795fb
8a9c0f8
0b5fb1f
962f16b
3b7c031
20118c6
037f1f7
ab3be62
b181c7c
acb2703
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thinking out loud - does the snapshot check belong here, or in the
EsqlVersion.parse
method? I can see arguments for both.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.
Hm, personally I do think it belongs here -
EsqlVersion
shouldn't need to worry about where we permit certain versions to be used IMHO, and why; also, this allows to keepEsqlVersion.parse
simple: either the string is a valid version or it's not - otherwise we'd have to throw an exception containing the reason why parsing failed and catch it inEsqlQueryRequest.validate()
. We might also parse versions coming from elsewhere in the future, and wouldn't want to prevent parsing of "nightly" just because we're not on a snapshot 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.
Better make this a class similar to
Version
- enum are quite restrictive and impossible to extend.Having a class gives us future extension points - in the worse case we won't use them and the class will be like an enum.
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.
I think we can flip it to a class with constants when we need that. If we can get away with an
enum
for a while I'd prefer that.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.
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.
I think I actually prefer an explicit throw here rather than just an assert. This should run once per instance of Elasticsearch, I don't think we need the performance gain of being able to disable asserts in production.
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.
I'm not entirely sure how
assert
works, but I'd be worried that putting these onto one line will make it not perform theput
if assertions are disabled. FWIW, it doesn't really matter if it runs it or not - it'll slow down a reader either way. But theassert
on it's own line is fin.You could make it a hard test. The extra test on startup is probably not going to hurt us.
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.
This suggestion looks cleaner, but is not equivalent; disabling assertions breaks the version parsing altogether as no version strings are added to the
stringToVersion
map anymore.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.
I'll add an explicit
throw new AssertionError(...)
.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.
If we think we might need to serialize this, we should explicitly pass in an ordinal value for serialization (that way it's proof against re-ordering the constants or deleting one). I don't know that we'll have to serialize these, but in case you aren't familiar with that pattern, I wanted to call it out.
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.
Discussed with Mark: will add a disambiguation counter for good measure, so versions will be
2024.04.01.🎉
and if we end up really having to release two language versions in a month, there may be a2024.04.02.💥
.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.
month
can be madebyte
.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.
Makes sense; I'd like to still use an
int
in the constructor. though, for easier readability if that's good with you?