-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Short-Circuit NOOP Mapping Updates Earlier #77574
Short-Circuit NOOP Mapping Updates Earlier #77574
Conversation
No need to actually run noop mapping updates or create a new document mapper if nothing has changed.
Pinging @elastic/es-distributed (Team:Distributed) |
Pinging @elastic/es-search (Team:Search) |
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, nice improvement @original-brownbear
return currentMapper; | ||
} | ||
synchronized (this) { | ||
Mapping incomingMapping = parseMapping(mappingType, mappingSource); |
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 can probably be outside the synchronized block as well?
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.
Technically I think so yes. But the parsing code does reference this.mapper
in some spots so I decided to keep it this way for easy to understand correctness. Will try to look into a follow-up to this that cleans up the synchronization here a little :)
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.
public PutMappingClusterStateUpdateRequest(String source) { | ||
this.source = source; | ||
public PutMappingClusterStateUpdateRequest(String source) throws IOException { | ||
this.source = new CompressedXContent(source); |
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.
Maybe we should take the action off the transport thread, seems like an action that do not need to run on transport and now that we compress, we do a bit (though tiny) of real work 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.
I thought about this, but it seemed like the wrong approach. Either, and this is true IMO, the compression cost is so tiny here that it doesn't matter really. Or if it does indeed matter, the better fix seems to be to just serialize the PutMappingRequest
with the mapping in compressed form right away (thus making it smaller and easier to read from the wire as well).
Thanks Alan & Henning! |
No need to actually run noop mapping updates or create a new document mapper if nothing has changed.
No need to actually run noop mapping updates or create a new document mapper
if nothing has changed. Also, we can do the compressing of the updates off of the master update
thread and make the updates slightly less heavy that way.