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

Adapt IndicesClusterStateService to use allocation ids #12397

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@bleskes
Member

bleskes commented Jul 22, 2015

#12242 introduced a unique id for an assignment of shard to a node. We should use these id's to drive the decisions made by IndicesClusterStateService when processing the new cluster state sent by the master. If the local shard has a different allocation id than the new cluster state, the shard will be removed and a new one will be created. This fixes a couple of subtle bugs, most notably a node previously got confused if an incoming cluster state had a newly allocated shard in the initializing state and the local copy was started (which can happen if cluster state updates are bulk processed). In that case, the node have previously re-used the local copy instead of initializing a new one.

Also, as set of utility methods was introduced on ShardRouting to do various types of matching with other shard routings, giving control about what exactly should be matched (same shard id, same allocation id, all but version and shard info etc.). This is useful here, but also prepares the grounds for the change needed in #12387 (making ShardRouting.equals be strict and perform exact equality).

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jul 22, 2015

Member

@kimchy @imotov can you have a look?

Member

bleskes commented Jul 22, 2015

@kimchy @imotov can you have a look?

Core: Adapt IndicesClusterStateService to use allocation ids
#12242 introduced a unique id for an assignment of shard to a node. We should use these id's to drive the decisions made by IndicesClusterStateService when processing the new cluster state sent by the master. If the local shard has a different allocation id than the new cluster state, the shard will be removed and a new one will be created. This fixes a couple of subtle bugs,  most notably a node previously got confused if an incoming cluster state had a newly allocated shard in the initializing state and the local copy was started (which can happen if cluster state updates are bulk processed). In that case, the node have previously re-used the local copy instead of initializing a new one.

 Also, as set of utility methods was introduced on ShardRouting to do various types of matching with other shard routings, giving control about what exactly should be matched (same shard id, same allocation id, all but version and shard info etc.). This is useful here, but also prepares the grounds for the change needed in #12387 (making ShardRouting.equals be strict and perform exact equality).
if (shardHasBeenRemoved == false) {
// shadow replicas do not support primary promotion. The master would reinitialize the shard, giving it a new allocation, meaning we should be there.
// nocommit: double check check this

This comment has been minimized.

@bleskes

bleskes Jul 22, 2015

Member

@dakrone can you please verify the above?

@bleskes

bleskes Jul 22, 2015

Member

@dakrone can you please verify the above?

This comment has been minimized.

@dakrone

dakrone Jul 22, 2015

Member

The previous behavior was that the service removed the shard if it was a shadow replica being promoted to primary, but it looks like this behavior was removed. Is it moved somewhere else?

@dakrone

dakrone Jul 22, 2015

Member

The previous behavior was that the service removed the shard if it was a shadow replica being promoted to primary, but it looks like this behavior was removed. Is it moved somewhere else?

This comment has been minimized.

@dakrone

dakrone Jul 22, 2015

Member

Okay, I tested this locally, it looks like it works differently with the line:

[2015-07-22 08:54:10,744][DEBUG][org.elasticsearch.indices.cluster] [node_t1] [test][0] removing shard (different instance of it allocated on this node, current [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [R], v[4], s[STARTED], a[id=93HrpsfMRWybeCn-NxhBjw]], global [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [P], v[6], s[INITIALIZING], a[id=g-oZ-7uiRTmDxgLnTdQHjg], unassigned_info[[reason=REINITIALIZED], at[2015-07-22T14:54:10.743Z]]])[2015-07-22 08:54:10,744][DEBUG][org.elasticsearch.indices.cluster] [node_t1] [test][0] removing shard (different instance of it allocated on this node, current [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [R], v[4], s[STARTED], a[id=93HrpsfMRWybeCn-NxhBjw]], global [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [P], v[6], s[INITIALIZING], a[id=g-oZ-7uiRTmDxgLnTdQHjg], unassigned_info[[reason=REINITIALIZED], at[2015-07-22T14:54:10.743Z]]])

Is my understanding correct?

@dakrone

dakrone Jul 22, 2015

Member

Okay, I tested this locally, it looks like it works differently with the line:

[2015-07-22 08:54:10,744][DEBUG][org.elasticsearch.indices.cluster] [node_t1] [test][0] removing shard (different instance of it allocated on this node, current [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [R], v[4], s[STARTED], a[id=93HrpsfMRWybeCn-NxhBjw]], global [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [P], v[6], s[INITIALIZING], a[id=g-oZ-7uiRTmDxgLnTdQHjg], unassigned_info[[reason=REINITIALIZED], at[2015-07-22T14:54:10.743Z]]])[2015-07-22 08:54:10,744][DEBUG][org.elasticsearch.indices.cluster] [node_t1] [test][0] removing shard (different instance of it allocated on this node, current [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [R], v[4], s[STARTED], a[id=93HrpsfMRWybeCn-NxhBjw]], global [[test][0], node[nNCDLISFRu2ncr-QCiYPgQ], [P], v[6], s[INITIALIZING], a[id=g-oZ-7uiRTmDxgLnTdQHjg], unassigned_info[[reason=REINITIALIZED], at[2015-07-22T14:54:10.743Z]]])

Is my understanding correct?

This comment has been minimized.

@bleskes

bleskes Jul 22, 2015

Member

yeah, my reasoning (which I wanted double checked) is that the master always generates a new allocation id in this case and thus the shard is cleaned by the allocation id check. No need to have special shadow replica handling (but I did add an assert and a comment).

@bleskes

bleskes Jul 22, 2015

Member

yeah, my reasoning (which I wanted double checked) is that the master always generates a new allocation id in this case and thus the shard is cleaned by the allocation id check. No need to have special shadow replica handling (but I did add an assert and a comment).

@imotov

This comment has been minimized.

Show comment
Hide comment
@imotov

imotov Jul 23, 2015

Member

Looks good to me. I have also rebased #12387 on this change and it no longer fails.

Member

imotov commented Jul 23, 2015

Looks good to me. I have also rebased #12387 on this change and it no longer fails.

@bleskes bleskes closed this in 316d1cb Jul 24, 2015

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jul 24, 2015

Member

thx @dakrone @imotov . pushed to master...

Member

bleskes commented Jul 24, 2015

thx @dakrone @imotov . pushed to master...

@bleskes bleskes deleted the bleskes:shard_matching branch Jul 24, 2015

@clintongormley clintongormley removed the review label Aug 7, 2015

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