Batch close-indices cluster state updates#84259
Conversation
rather than passing it in
|
Hi @joegallo, I've created a changelog YAML for you. |
DaveCTurner
left a comment
There was a problem hiding this comment.
Looks good, I left a few nits but seems structurally sound to me.
| this.indicesService = indicesService; | ||
| this.shardLimitValidator = shardLimitValidator; | ||
| } | ||
| private class OpenIndicesExecutor implements ClusterStateTaskExecutor<OpenIndicesTask> { |
There was a problem hiding this comment.
Slight preference to keep this (and other executors) as static classes that hold onto the services they need directly.
There was a problem hiding this comment.
Ehhhh, this is a bit of a pain -- private class WaitForClosedBlocksApplied / private class WaitForBlocksApplied, and the former is invoked by AddBlocksToCloseExecutor, so I'd have to walk those two back, too.
I'm really with you on disliking (non-static) inner classes, they feel a bit too 'more magic' for me, but in this case they're all private anyway, and the juggling of the fields across them all just seems like noise.
That said, this isn't 'no', it's just 'ehhh, this is ever so slightly harder than you might have thought' -- feel free to say something like "yeah, that's cool, please make the change anyway" and I will happily do it.
There was a problem hiding this comment.
My preference is only slight, I'll defer to your judgement.
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
Since we're just building a list with the returned collection anyway, we might as well internalize that since this is the only production invocation anyway. Changed finalizeBlock, too, for parity, although that doesn't have the full benefit here just yet because it hasn't been task-ified.
original-brownbear
left a comment
There was a problem hiding this comment.
LGTM (nits really can be ignored, just had to let them out :))
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-data-management (Team:Data Management) |
Related to #81627 and #83432, followup to #83760.