Write state also on data nodes if not master eligible #9952
Conversation
| // remove the index state for this index if it is only a data node | ||
| // only delete if the last shard was removed | ||
| if (shardsAllocatedOnThisNodeInLastClusterState) { | ||
| removeIndexState(indexMetaData); |
There was a problem hiding this comment.
I am not sure we need this removal logic, we already have that in IndicesClusterStateService#applyCleanedIndices, where an index is removed is no shards are around any more, its nice that it would be in a single place?
There was a problem hiding this comment.
ohh, I think I see what happens, in IndicesClusterStateService, we just remove the index and not delete it from the file system, which is good. In IndicesStore, only there we actually delete the shard content once the shard is allocated on all the other nodes. I think that we need to add a logic there that if the there are no more shards around, we need to also delete the index itself (which will delete its metadata).
|
I left some comments... |
|
Made a pr for the deletion of index folders here: #9985 Should be easy to remove all the additional deletion code from this pr. |
brwe
force-pushed
the
data-node-state-write-pr
branch
2 times, most recently
from
fcbeaed
to
011f424
Compare
|
#9985 is merged, I rebased on latest master and changed the code accordingly. I wanted to remove the change in ClusterStateEvent also because I was unable to reproduce the failures I'd seen before without it. But now I found that without the change the tests only pass on my Linux machine but fail every 10 iterations or so on my mac so something is still fishy. I'll try to come up with a detailed failure analysis tomorrow. |
|
I think I know what is going on now: The fresh master with the empty cluster state does (rarely) not send the first cluster state due to a race condition in lifecycles of DiscoveryService and its member Discovery. In DiscoveryService.doStart() the Discovery is started but the lifecyle for DiscoveryService is started only after that. This is why when the first cluster state reaches DiscoveryService.publish the lifecycle might or might not have started. I added a commit d69f2cf where I removed the ClusterStateEvent change and added an artificial delay to the DiscoveryService.doStart() so that the tests fail reliably just so you can check if you want. I would suggest we remove the ClusterStateEvent workaround and open another issue for this because this behavior is not a result of this pull request. |
|
I agree with your idea of opening a new issue for the ClusterSTateEvent problem |
brwe
force-pushed
the
data-node-state-write-pr
branch
2 times, most recently
from
547de44
to
72f9f9f
Compare
| @@ -90,6 +94,26 @@ public MetaData loadMetaState() throws Exception { | |||
| return metaStateService.loadFullState(); | |||
| } | |||
|
|
|||
| public static class IndexMetaWriteInfo { | |||
| IndexMetaData newMetaData; | |||
There was a problem hiding this comment.
can they all be package private and final please? just like a struct
There was a problem hiding this comment.
also you might wanna put this to the end of the file
also, deal with closed indices also on data nodes
brwe
force-pushed
the
data-node-state-write-pr
branch
from
6e63466
to
7569a51
Compare
When a node was a data node only then the index state was not written. In case this node connected to a master that did not have the index in the cluster state, for example because a master was restarted and the data folder was lost, then the indices were not imported as dangling but instead deleted. This commit makes sure that index state for data nodes is also written if they have at least one shard of this index allocated. closes #8823 closes #9952
|
need to investigate #10017 before we can push |
|
The reason why the tests failed on CI is the same I described in the beginning #9952 (comment) : a data node receives a new cluster state from a master that does not have the index in its state but the data node missed the state with a no master block before and so state persistence was not disabled. the fact that an index is not in the cluster state is then interpreted as delete command. This can happen here for the reasons described in #10017 but there might be other reasons as well. I now think we should not delete indices at all if the cluster state that would cause a deletion comes from a new master. |
…n global state changed
+1 to the solution |
|
Chatted with @kimchy and we decided to push as is and add a //norelease comment and open an issue because the short term fix for the problem (#9952 (comment)) is not very elegant. |
|
LGTM |
clintongormley
added
:Distributed/Distributed
When a node was a data node only then the index state was not written.
In case this node connected to a master that did not have the index
in the cluster state, for example because a master was restarted and
the data folder was lost, then the indices were not imported as dangling
but instead deleted.
This commit makes sure that index state for data nodes is also written
if they have at least one shard of this index allocated.
I am a little lost with this. I found that the index can still be deleted
from a data node if the state was written but the node gets a new cluster state from a
master that does not have it, for example because it was restarted without data folder. Happens
if the data node does not get the initial cluster state from the new but a later one and state
persistence is not disabled.
I avoid this now by this: https://github.com/elasticsearch/elasticsearch/pull/9952/files#diff-f0f71bedb3d7e6f1cec54e8dddf5c3d3R109
but am worried about side effects this might have. Any feedback appreciated.
closes #8823