Supporting automatic release of index blocks. Closes #39334 #40338
Conversation
DaveCTurner
added
>enhancement
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed |
alex101101
force-pushed
the
automatic-release-of-index-blocks
branch
from
0abf7b2 to
906dbe9
Compare
There was a problem hiding this comment.
Thanks @alex101101. My main concern with this implementation is that it might release the block if a node that's above the flood stage threshold failed to respond to the stats request sent by the InternalClusterInfoService, because then it wouldn't be included in the disk usage statistics. I think it'd be better to only release this block if we are certain that every assigned shard is definitely on a node below the threshold.
I left a few more comments on the implementation, but I haven't been through the tests in detail yet.
| nodes that it is sharded across are both 5% above the flood stage watermark | ||
| and above the high watermark. | ||
|
|
||
| `cluster.routing.allocation.disk.auto_release_index_enabled`:: |
There was a problem hiding this comment.
I think that we should move towards making this the default behaviour. I cannot really think of a case where the current behaviour is preferable. We may want to introduce a system property for the 7.x series just in case this counts as a breaking change, similar to this code in 6.7:
I'm going to check this with the team to be certain that there are no objections.
There was a problem hiding this comment.
I'm going to check this with the team to be certain that there are no objections.
We discussed this today and agreed to make this the only behaviour in future: this should be the only option in 8.0 and should be the usual behaviour in 7.x, although we would support disabling it with a system property, es.auto_release_flood_stage_block in 7.x.
| RoutingNode routingNode = state.getRoutingNodes().node(node); | ||
|
|
||
| // Only unblock index if all nodes that contain shards of it are above the node auto release threshold | ||
| if (nodeIndicesAvailableForAutoRelease(usage)) { |
There was a problem hiding this comment.
I think I'd like this nodeIndicesAvailableForAutoRelease to be inlined to keep it in harmony with the rest of this method.
| Set<String> indicesToDisableReadOnly = autoReleaseEligibility.entrySet().stream() | ||
| .filter(Map.Entry::getValue) | ||
| .map(Map.Entry::getKey) | ||
| .filter(index -> state.getBlocks().indexBlocked(ClusterBlockLevel.WRITE, index)) |
There was a problem hiding this comment.
I think we should only be contemplating removing the block if the index has the specific index-level READ_ONLY_ALLOW_DELETE block that we add here.
| client.admin().indices().prepareUpdateSettings(indicesToMarkReadOnly.toArray(Strings.EMPTY_ARRAY)). | ||
| setSettings(Settings.builder().put(IndexMetaData.SETTING_READ_ONLY_ALLOW_DELETE, true).build()).execute(); | ||
| client.admin().indices().prepareUpdateSettings(indicesToUpdate.toArray(Strings.EMPTY_ARRAY)). | ||
| setSettings(Settings.builder().put(IndexMetaData.SETTING_READ_ONLY_ALLOW_DELETE, readOnly).build()).execute(); |
There was a problem hiding this comment.
I think we should remove the setting completely from the index metadata, not just set it to false as done here.
| } | ||
|
|
||
| private boolean canSetAutoRelease() { | ||
| return (this.freeBytesThresholdHigh != null && this.freeBytesThresholdFloodStage != null); |
There was a problem hiding this comment.
Is this needed? I can't see that this.freeBytesThresholdHigh is ever null, and this.freeBytesThresholdFloodStage should only be null during initialisation, which I think I'd prefer to fix by setting it to new ByteSizeValue(0, ByteSizeUnit.BYTES) first.
|
Also please don't force-push to PRs, it makes it much harder to keep track things. |
|
Superseded by #42559. |
Automatically remove INDEX_READ_ONLY_ALLOW_DELETE_BLOCK block on index once node(s) disk usage at least 5% below the flood_stage watermark and below the high watermark. Feature disabled by default. Closes #39334