-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Allow RerouteService to reroute at lower priority #44338
Allow RerouteService to reroute at lower priority #44338
Conversation
Today the `BatchedRerouteService` submits its delayed reroute task at `HIGH` priority, but in some cases a lower priority would be more appropriate. This commit adds the facility to submit delayed reroute tasks at different priorities, such that each submitted reroute task runs at a priority no lower than the one requested. It does not change the fact that all delayed reroute tasks are submitted at `HIGH` priority, but at least it makes this explicit.
Pinging @elastic/es-distributed |
Jenkins run elasticsearch-ci/packaging-sample |
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.
@DaveCTurner I did a first quick pass and it looks like this makes the listener invocation behave differently (and unsafe).
@@ -95,7 +123,7 @@ public void onNoLongerMaster(String source) { | |||
pendingRerouteListeners = null; | |||
} | |||
} | |||
currentListeners.onFailure(new NotMasterException("delayed reroute [" + reason + "] cancelled")); | |||
currentListeners.forEach(l -> l.onFailure(new NotMasterException("delayed reroute [" + reason + "] cancelled"))); |
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 use org.elasticsearch.action.ActionListener#onFailure(java.lang.Iterable<org.elasticsearch.action.ActionListener<Response>>, java.lang.Exception)
to make this a little safer?
@@ -114,22 +142,26 @@ public void onFailure(String source, Exception e) { | |||
logger.error(() -> new ParameterizedMessage("unexpected failure during [{}], current state version [{}]", | |||
source, state.version()), e); | |||
} | |||
currentListeners.onFailure(new ElasticsearchException("delayed reroute [" + reason + "] failed", e)); | |||
currentListeners.forEach(l -> |
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, use org.elasticsearch.action.ActionListener#onFailure(java.lang.Iterable<org.elasticsearch.action.ActionListener<Response>>, java.lang.Exception)
?
} | ||
|
||
@Override | ||
public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { | ||
currentListeners.onResponse(null); | ||
currentListeners.forEach(l -> l.onResponse(null)); |
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 issue as with the failures here as well. We probably should use org.elasticsearch.action.ActionListener#onResponse(java.lang.Iterable<org.elasticsearch.action.ActionListener<Response>>, Response)
to not fail the loop on the first exception? (Also, this makes the behavior equivalent to what it was where PlainListenableActionFuture
would do the same and ensure all the listeners are called).
Good point, thanks @original-brownbear. Fixed in 98796c4. |
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.
LGMT, thanks @DaveCTurner !
Today the `BatchedRerouteService` submits its delayed reroute task at `HIGH` priority, but in some cases a lower priority would be more appropriate. This commit adds the facility to submit delayed reroute tasks at different priorities, such that each submitted reroute task runs at a priority no lower than the one requested. It does not change the fact that all delayed reroute tasks are submitted at `HIGH` priority, but at least it makes this explicit.
Today the
BatchedRerouteService
submits its delayed reroute task atHIGH
priority, but in some cases a lower priority would be more appropriate. This
commit adds the facility to submit delayed reroute tasks at different
priorities, such that each submitted reroute task runs at a priority no lower
than the one requested. It does not change the fact that all delayed reroute
tasks are submitted at
HIGH
priority, but at least it makes this explicit.