-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fix testBadAsyncId #132705
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
Fix testBadAsyncId #132705
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| public static final ParseField DETAILS_FIELD = new ParseField("details"); | ||
| public static final ParseField TOOK = new ParseField("took"); | ||
| public static final ParseField IS_PARTIAL_FIELD = new ParseField("is_partial"); | ||
| private static final Logger log = LogManager.getLogger(EsqlExecutionInfo.class); |
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 assume the log was for debugging? We probably don't need it anymore here?
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.
Oh, yes, that is correct, let me remove it
| out.writeCollection(clusterInfo.values()); | ||
| // .stream().toList() creates an immutable copy of the cluster info entries | ||
| // as today they might be still changing while serialization is happening | ||
| out.writeCollection(clusterInfo.values().stream().toList()); |
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 am pretty sure this will fix it, but I am not entirely happy about this because the only place where this is needed is when we're initializing the cluster list, which is a very brief moment in the life of the query, but we're paying for it with having to copy this structure every time, even though most of the time it's not needed. I wonder if there's a better way.
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.
Possibly. Lets merge this fix to address the issue and I will take a look if we could avoid underlying concurrent modification as part of another change.
See #132353 (comment) for the failure explanation.
Related to: #132353