Skip to content
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

Tighten mapping syncing in ccr remote restore #38071

Merged
merged 3 commits into from
Feb 4, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jan 31, 2019

There are two issues regarding the way that we sync mapping from leader to follower when a ccr restore is completed:

  1. The returned mapping from a cluster service might not be up to date as the mapping of the restored index commit.
  2. We should not compare the mapping version of the follower and the leader. They are not related to one another.

Moreover, I think we should only ensure that once the restore is done, the mapping on the follower should be at least the mapping of the copied index commit. We don't have to sync the mapping which is updated after we have opened a session.

Relates #36879
Closes #37887

@dnhatn dnhatn added >enhancement v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.7.0 labels Jan 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dnhatn

@@ -433,10 +424,6 @@ public void testFollowerMappingIsUpdated() throws IOException {
clusterStateRequest.clear();
clusterStateRequest.metaData(true);
clusterStateRequest.indices(followerIndex);
ClusterStateResponse clusterState = followerClient().admin().cluster().state(clusterStateRequest).actionGet();
IndexMetaData followerIndexMetadata = clusterState.getState().metaData().index(followerIndex);
assertEquals(2, followerIndexMetadata.getMappingVersion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we changed mapping twice in the leader: one before opening session and one after. These two events are propagated separately; then the mapping version on the follower is 2. Now, these two events happen before we open session, then the mapping version on the follower is 1 instead of 2. I think verifying the content of the mapping is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnhatn
Copy link
Member Author

dnhatn commented Feb 4, 2019

run elasticsearch-ci/2

@dnhatn
Copy link
Member Author

dnhatn commented Feb 4, 2019

Thanks everyone.

@dnhatn dnhatn merged commit cecfa5b into elastic:master Feb 4, 2019
@dnhatn dnhatn deleted the remote-recovery-mapping branch February 4, 2019 22:53
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 5, 2019
…-lease-expiration

* elastic/master: (24 commits)
  Add support for API keys to access Elasticsearch (elastic#38291)
  Add typless client side GetIndexRequest calls and response class (elastic#37778)
  Limit token expiry to 1 hour maximum (elastic#38244)
  add docs saying mixed-cluster ILM is not supported (elastic#37954)
  Skip unsupported languages for tests (elastic#38328)
  Deprecate `_type` in simulate pipeline requests (elastic#37949)
  Mute testCannotShrinkLeaderIndex (elastic#38374)
  Tighten mapping syncing in ccr remote restore (elastic#38071)
  Add test for `PutFollowAction` on a closed index (elastic#38236)
  Fix SSLContext pinning to TLSV1.2 in reload tests (elastic#38341)
  Mute RareClusterStateIT.testDelayedMappingPropagationOnReplica (elastic#38357)
  Deprecate types in rollover index API (elastic#38039)
  Types removal - fix FullClusterRestartIT warning expectations (elastic#38310)
  Fix ILM explain response to allow unknown fields (elastic#38054)
  Mute testFollowIndexAndCloseNode (elastic#38360)
  Docs: Drop inline callout from scroll example (elastic#38340)
  Deprecate HLRC security methods (elastic#37883)
  Remove types from Monitoring plugin "backend" code (elastic#37745)
  Add Composite to AggregationBuilders (elastic#38207)
  Clarify slow cluster-state log messages (elastic#38302)
  ...
Tim-Brooks pushed a commit to Tim-Brooks/elasticsearch that referenced this pull request Feb 5, 2019
There are two issues regarding the way that we sync mapping from leader
to follower when a ccr restore is completed:

1.  The returned mapping from a cluster service might not be up to date
as the mapping of the restored index commit.

2. We should not compare the mapping version of the follower and the
leader. They are not related to one another.

Moreover, I think we should only ensure that once the restore is done,
the mapping on the follower should be at least the mapping of the copied
index commit. We don't have to sync the mapping which is updated after
we have opened a session.

Relates elastic#36879
Closes elastic#37887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >enhancement v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants