New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make shard store fetch less dependent on the current cluster state, both on master and non data nodes #19044

Merged
merged 11 commits into from Jun 27, 2016

Conversation

Projects
None yet
3 participants
@bleskes
Member

bleskes commented Jun 23, 2016

#18938 has changed the timing in which we send out to nodes to fetch their shard stores. Instead of doing this after the cluster state resulting of the node's join was published, #18938 made it be sent concurrently to the publishing processes. This revealed a couple of points where the shard store fetching is dependent of the current state of affairs of the cluster state, both on the master and the data nodes. The problem discovered were already present without #18938 but required a failure/extreme situations to make them happen.This PR tries to remove as much as possible of these dependencies making shard store fetching simpler and make the way to re-introduce #18938 which was reverted.

These are the notable changes:

  1. Allow TransportNodesAction (of which shard store fetching is derived) callers to supply concrete disco nodes, so it won't need the cluster state to resolve them. This was a problem because the cluster state containing the needed nodes was not yet made available through ClusterService. Note that long term we can expect the rest layer to resolve node ids to concrete nodes, making this mode the only one needed.
  2. The data node relied on the cluster state to have the relevant index meta data so it can find data when custom paths are used. We now fall back to read the meta data from disk if needed.
  3. The data node was relying on it's own IndexService state to indicate whether the data it has corresponds to an existing allocation. This is of course something it can not know until it got (and processed) the new cluster state from the master. This flag in the response is now removed. This is not a problem because we used that flag to protect against double assigning of a shard to the same node, but we are already protected from it by the allocation deciders.
  4. I removed the redundant filterNodeIds method in TransportNodesAction - if people want to filter they can override resolveRequest.
@@ -102,8 +102,15 @@ public boolean processExistingRecoveries(RoutingAllocation allocation) {
if (matchingNodes.getNodeWithHighestMatch() != null) {
DiscoveryNode currentNode = allocation.nodes().get(shard.currentNodeId());
DiscoveryNode nodeWithHighestMatch = matchingNodes.getNodeWithHighestMatch();
// current node will not be in matchingNodes as it is filtered away by SameShardAllocationDecider

This comment has been minimized.

@ywelsch

ywelsch Jun 24, 2016

Contributor

good catch!

@ywelsch

ywelsch Jun 24, 2016

Contributor

good catch!

currentSyncId = shardStores.getData().get(currentNode).storeFilesMetaData().syncId();
} else {
currentSyncId = null;
}
if (currentNode.equals(nodeWithHighestMatch) == false

This comment has been minimized.

@ywelsch

ywelsch Jun 24, 2016

Contributor

Is this a tautology?

@ywelsch

ywelsch Jun 24, 2016

Contributor

Is this a tautology?

This comment has been minimized.

@bleskes

bleskes Jun 25, 2016

Member

not sure I follow - do you mean the current node is always nodeWithHighestMatch?

@bleskes

bleskes Jun 25, 2016

Member

not sure I follow - do you mean the current node is always nodeWithHighestMatch?

This comment has been minimized.

@ywelsch

ywelsch Jun 27, 2016

Contributor

The current node can never be nodeWithHighestMatch as findMatchingNodes will not add current node due to SameShardAllocationDecider. This means that currentNode.equals(nodeWithHighestMatch) == false always holds and can thus be removed from the condition (or you can add it as assertion).

@ywelsch

ywelsch Jun 27, 2016

Contributor

The current node can never be nodeWithHighestMatch as findMatchingNodes will not add current node due to SameShardAllocationDecider. This means that currentNode.equals(nodeWithHighestMatch) == false always holds and can thus be removed from the condition (or you can add it as assertion).

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

that's true. ++ on an assertion

@bleskes

bleskes Jun 27, 2016

Member

that's true. ++ on an assertion

@@ -157,15 +158,17 @@ protected static ClusterState startRandomInitializingShard(ClusterState cluster
}
protected static AllocationDeciders yesAllocationDeciders() {
return new AllocationDeciders(Settings.EMPTY, new AllocationDecider[] {new TestAllocateDecision(Decision.YES)});
return new AllocationDeciders(Settings.EMPTY, new AllocationDecider[] {new TestAllocateDecision(Decision.YES),

This comment has been minimized.

@ywelsch

ywelsch Jun 24, 2016

Contributor

was this change required to prevent some tests from failing?

@ywelsch

ywelsch Jun 24, 2016

Contributor

was this change required to prevent some tests from failing?

This comment has been minimized.

@bleskes

bleskes Jun 25, 2016

Member

yeah, without it the assertions in the routing nodes universe complain that a shard is assigned to a node that already has it. I do think that we should hard code this allocation decider (and the one with replica follow primary) into the set of deciders so it will always be there. It's an integral part of how our system works

@bleskes

bleskes Jun 25, 2016

Member

yeah, without it the assertions in the routing nodes universe complain that a shard is assigned to a node that already has it. I do think that we should hard code this allocation decider (and the one with replica follow primary) into the set of deciders so it will always be there. It's an integral part of how our system works

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Jun 24, 2016

Contributor

Left minor comments. Also there is a word missing in the title ;-)

Contributor

ywelsch commented Jun 24, 2016

Left minor comments. Also there is a word missing in the title ;-)

@bleskes bleskes changed the title from Make shard store fetch less dependent on the current cluster state, both on master and non nodes to Make shard store fetch less dependent on the current cluster state, both on master and non data nodes Jun 24, 2016

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jun 25, 2016

Member

@ywelsch thx. I pushed another commit

Member

bleskes commented Jun 25, 2016

@ywelsch thx. I pushed another commit

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Jun 27, 2016

Contributor

Left one more comment (to get rid of a line of code). Feel free to push once addressed. LGTM

Contributor

ywelsch commented Jun 27, 2016

Left one more comment (to get rid of a line of code). Feel free to push once addressed. LGTM

@bleskes bleskes merged commit cb0824e into elastic:master Jun 27, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@bleskes bleskes deleted the bleskes:async_fetch_use_nodes branch Jun 27, 2016

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jun 27, 2016

Member

thanks @ywelsch ! I added the assertion and pushed.

Member

bleskes commented Jun 27, 2016

thanks @ywelsch ! I added the assertion and pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment