Omit writing index metadata for non-replicated closed indices on data-only node #47285
Conversation
ywelsch
added
>bug
:Distributed Coordination/Cluster Coordination
|
Pinging @elastic/es-distributed |
ywelsch
changed the title
| @@ -244,10 +229,10 @@ private long writeGlobalState(AtomicClusterStateWriter writer, MetaData newMetaD | |||
| } | |||
|
|
|||
| // exposed for tests | |||
| static Set<Index> getRelevantIndices(ClusterState state, ClusterState previousState, Set<Index> previouslyWrittenIndices) { | |||
| static Set<Index> getRelevantIndices(ClusterState state) { | |||
| Set<Index> relevantIndices; | |||
| if (isDataOnlyNode(state)) { | |||
There was a problem hiding this comment.
Seeing as how we're touching this, if we checked state.nodes().getLocalNode().isMasterNode() first then we could say merely else if (state.nodes().getLocalNode(). isDataOnlyNode()) and ditch the isDataOnlyNode method.
And there's no need for the intermediate relevantIndices, just return the thing already :)
| @@ -207,8 +207,7 @@ private long writeGlobalState(AtomicClusterStateWriter writer, MetaData newMetaD | |||
| return actions; | |||
| } | |||
|
|
|||
| private static Set<Index> getRelevantIndicesOnDataOnlyNode(ClusterState state, ClusterState previousState, Set<Index> | |||
| previouslyWrittenIndices) { | |||
| private static Set<Index> getRelevantIndicesOnDataOnlyNode(ClusterState state) { | |||
server/src/test/java/org/elasticsearch/gateway/IncrementalClusterStateWriterTests.java
Outdated
Show resolved
Hide resolved
| clusterStateWithJustOpenedIndex(indexMetaData, false), | ||
| clusterStateWithClosedIndex(indexMetaData, false), | ||
| Collections.emptySet()); | ||
| clusterStateWithJustOpenedIndex(indexMetaData, false)); | ||
| assertThat(indices.size(), equalTo(0)); | ||
| } | ||
|
|
||
| public void testGetRelevantIndicesForWasClosedPrevWrittenIndexOnDataOnlyNode() { | ||
| IndexMetaData indexMetaData = createIndexMetaData("test"); |
There was a problem hiding this comment.
This test is now a duplicate of the previous one, and I think that PrevNotWritten/PrevWritten becomes misleading now getRelevantIndicesOnDataOnlyNode is changed. I'd prefer to have testGetRelevantIndicesForClosedIndexOnDataOnlyNode testing a closed index replicated/not yet replicated
There was a problem hiding this comment.
makes sense, updated
| .build()); | ||
| indexRandom(randomBoolean(), randomBoolean(), randomBoolean(), IntStream.range(0, randomIntBetween(0, 50)) | ||
| .mapToObj(n -> client().prepareIndex(indexName, "_doc").setSource("num", n)).collect(toList())); | ||
| assertAcked(client().admin().indices().prepareClose(indexName)); |
There was a problem hiding this comment.
Don't forget the waitForActiveShards when backporting :) (I always forgot it)
|
For posterity, we discussed the various implications of this change. It means that we stop updating the on-disk index metadata for shards of closed non-replicated indices on master-ineligible data nodes. (Master-eligible nodes persist the metadata of all indices in the cluster state, and replicated closed indices appear in the routing table). The index metadata stored on the disk of a master-ineligible data node is read when the node starts up and also when importing a dangling index. The metadata is read at startup so we can fail the node immediately if it has some invalid/unreadable index metadata but otherwise this has no effect since it is superseded by fresher metadata from the master at join time. It's pretty weak as a corruption check (it doesn't detect non-metadata corruption) and doesn't help much in terms of ensuring the node is compatible with all the indices currently in the cluster, so we're ok with losing that. When importing a dangling index we already have no freshness guarantees. Furthermore the shard data of a closed index cannot change while that index is closed or unassigned, so we do not need to keep the metadata up to date in order to be sure we will be able to import it as a dangling index in the future. With this change there'll be more chance that a dangling index import will see older mappings or settings, but they will never be so old that the shard is unreadable. |
…-only node (#47285) Fixes a bug related to how "closed replicated indices" (introduced in 7.2) interact with the index metadata storage mechanism, which has special handling for closed indices (but incorrectly handles replicated closed indices). On non-master-eligible data nodes, it's possible for the node's manifest file (which tracks the relevant metadata state that the node should persist) to become out of sync with what's actually stored on disk, leading to an inconsistency that is then detected at startup, refusing for the node to start up. Closes #47276
…-only node (#47285) Fixes a bug related to how "closed replicated indices" (introduced in 7.2) interact with the index metadata storage mechanism, which has special handling for closed indices (but incorrectly handles replicated closed indices). On non-master-eligible data nodes, it's possible for the node's manifest file (which tracks the relevant metadata state that the node should persist) to become out of sync with what's actually stored on disk, leading to an inconsistency that is then detected at startup, refusing for the node to start up. Closes #47276
Fixes a bug related to how "closed replicated indices" (introduced in 7.2) interact with the index metadata storage mechanism, which has special handling for closed indices (but incorrectly handles replicated closed indices). On non-master-eligible data nodes, it's possible for the node's manifest file (which tracks the relevant metadata state that the node should persist) to become out of sync with what's actually stored on disk, leading to an inconsistency that is then detected at startup, refusing for the node to start up.
The solution used here is to remove the code that treats closed indices specially. This code has not aged well, and its use is dubious as best.
Closes #47276