Fix messaging about delayed allocation #12515
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously when RoutingService checks for delayed shards, it can see shards that are delayed, but are past their delay time so the logged output looks like: ``` delaying allocation for [0] unassigned shards, next check in [0s] ``` This change allows shards that have passed their delay to be counted correctly for the logging. Additionally, it places a 5 second minimum delay between scheduled reroutes to try to minimize the number of reroutes run. This also adds a test that creates a large number of unassigned delayed shards and ensures that they are rerouted even if a single reroute does not allocated all shards (due to a low concurrent_recoveries setting). Resolves elastic#12456
| TimeValue nextDelay = TimeValue.timeValueMillis(UnassignedInfo.findNextDelayedAllocationIn(settings, event.state())); | ||
| long nextDelayMillis = UnassignedInfo.findNextDelayedAllocationIn(settings, event.state()); | ||
| // Schedule the delay at least 5 seconds in the future | ||
| nextDelayMillis = Math.max(5000, nextDelayMillis); |
There was a problem hiding this comment.
perhaps the minimum delay should be a setting?
There was a problem hiding this comment.
Sure, I will make it a non-dynamic configurable setting.
|
LGTM. Left a non blocking question. |
| @@ -190,7 +190,7 @@ public static int getNumberOfDelayedUnassigned(Settings settings, ClusterState s | |||
| if (shard.primary() == false) { | |||
| IndexMetaData indexMetaData = state.metaData().index(shard.getIndex()); | |||
| long delay = shard.unassignedInfo().getDelayAllocationExpirationIn(settings, indexMetaData.getSettings()); | |||
| if (delay > 0) { | |||
| if (delay != 0) { | |||
There was a problem hiding this comment.
So negative delays count delays? I figured they should count as not-delayed.
There was a problem hiding this comment.
Ah! I get it! 0 means they are not delayed at all but a negative number means they've already expired. They aren't delayed any more. OK - I retract my question. Maybe a comment?
|
LGTM |
|
Closing this, handled a different way in #12532 |
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Aug 5, 2015
In order to avoid extra reroutes, `RoutingService` should avoid scheduling a reroute of any shards where the delay is negative. To make sure that we don't encounter a race condition between the GatewayAllocator thinking a shard is delayed and RoutingService thinking it is not, the GatewayAllocator will update the RoutingService with the last time it checked in order to use a consistent "view" of the delay. Resolves elastic#12456 Relates to elastic#12515 and elastic#12456
dakrone
added a commit
to dakrone/elasticsearch
that referenced
this pull request
Aug 5, 2015
In order to avoid extra reroutes, `RoutingService` should avoid scheduling a reroute of any shards where the delay is negative. To make sure that we don't encounter a race condition between the GatewayAllocator thinking a shard is delayed and RoutingService thinking it is not, the GatewayAllocator will update the RoutingService with the last time it checked in order to use a consistent "view" of the delay. Resolves elastic#12456 Relates to elastic#12515 and elastic#12456
lcawl
added
:Distributed/Distributed
clintongormley
added
:Distributed/Allocation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously when RoutingService checks for delayed shards, it can see
shards that are delayed, but are past their delay time so the logged
output looks like:
This change allows shards that have passed their delay to be counted
correctly for the logging. Additionally, it places a 5 second minimum
delay between scheduled reroutes to try to minimize the number of
reroutes run.
This also adds a test that creates a large number of unassigned delayed
shards and ensures that they are rerouted even if a single reroute does
not allocated all shards (due to a low concurrent_recoveries setting).
Resolves #12456
(This PR is against 1.7 and will be forward-ported)