-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ESQL] Add informative timestamps to ESQL async #137957
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] Add informative timestamps to ESQL async #137957
Conversation
|
@nik9000 @alex-spies if you have some time, could you give this a quick look to figure out if this draft would look good to you? @AlexGPlay of @elastic/kibana-data-discovery in a exploration phase created this draft, that would help us with our background search effort 🙏 |
luigidellaquila
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.
Thanks @AlexGPlay
It looks good in general, but it needs some BWC checks, see my comments below.
| long valuesLoaded = supportsValuesLoaded(in.getTransportVersion()) ? in.readVLong() : 0; | ||
| Profile profile = in.readOptionalWriteable(Profile::readFrom); | ||
| boolean columnar = in.readBoolean(); | ||
| long startTimeMillis = in.readLong(); |
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 needs a TransportVersion check, for bwc.
You'll need to define a new TransportVersion constant (see this) and then you'll have to add a check like
if(in.getTransportVersion().supports(YOUR_NEW_CONSTANT_NAME_HERE)) {
// ... read the values
} else {
// ... some defaults...?
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.
thank you! I'll update with this
| } | ||
| out.writeOptionalWriteable(profile); | ||
| out.writeBoolean(columnar); | ||
| out.writeLong(startTimeMillis); |
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 here, this needs a TransportVersion check
|
@luigidellaquila i've added the TransportVersion and moved all the fields behind a check for the response in case they are not populated, happy to update if that's not the expected pattern 494a55f |
|
Thanks @AlexGPlay, the implementation looks good. |
luigidellaquila
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, thanks @AlexGPlay!
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Closes #137672
Run from Kibana:
gradle check?