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.

Sign up for GitHub

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

Jump to bottom

Push primary term to replication tracker #38044

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Push primary term to replication tracker #38044

merged 4 commits into from
Jan 31, 2019

Conversation

jasontedor
Copy link
Member

This commit pushes the primary term into the replication tracker. This is a precursor to using the primary term to resolving ordering problems for retention leases. Namely, it can be that out-of-order retention lease sync requests arrive on a replica. To resolve this, we need a tuple of (primary term, version). For this to be, the primary term needs to be accessible in the replication tracker. As the primary term is part of the replication group anyway, this change conceptually makes sense.

Relates #37951
Relates #38036

@jasontedor
Push primary term to replication tracker
5d11aab 
This commit pushes the primary term into the replication tracker. This
is a precursor to using the primary term to resolving ordering problems
for retention leases. Namely, it can be that out-of-order retention
lease sync requests arrive on a replica. To resolve this, we need a
tuple of (primary term, version). For this to be, the primary term needs
to be accessible in the replication tracker. As the primary term is part
of the replication group anyway, this change conceptually makes sense.
@jasontedor jasontedor added v7.0.0 :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >refactoring v6.7.0 labels Jan 30, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

dnhatn
dnhatn reviewed Jan 30, 2019
View reviewed changes
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

As the primary term is part of the replication group anyway, this change conceptually makes sense.

This makes sense to me too. I thought we are going to provide a PrimaryTermSupplier to ReplicationTracker and I find that having pendingPrimaryTerm and operationPrimaryTerm at the same place is a bit easier to understand than two places.

This change looks good to me, but I prefer to wait for @ywelsch on this.

ywelsch
ywelsch approved these changes Jan 30, 2019
View reviewed changes
server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java Outdated
*
* @return the primary term
*/
public long getOperationPrimaryTerm() {
Copy link
Contributor

Choose a reason for hiding this comment

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

please move getter and setter further down. I understand this is your preferred coding style. However, it conflicts with the coding style we have adapted for all these classes and don't think this is the right time and place to switch things up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed 72e2e7b.

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Outdated
operationPrimaryTerm = pendingPrimaryTerm;
final long primaryTerm = indexSettings.getIndexMetaData().primaryTerm(shardId.id());
this.pendingPrimaryTerm = primaryTerm;
replicationTracker.setOperationPrimaryTerm(primaryTerm);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we set this in the constructor of ReplicationTracker? Then we would always have a proper operation term in ReplicationTracker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed 72e2e7b.

@jasontedor
Merge branch 'master' into replication-tracker-primary-term
0895a5c 
* master:
  Remove types from watcher docs (elastic#38002)
  Add test coverage for Painless general casting of boolean and Boolean (elastic#37780)
  Fixed test bug, lastFollowTime is null if there are no follower indices.
  Add ECS schema for user-agent ingest processor (elastic#37727) (elastic#37984)
  Extract TransportRequestDeduplication from ShardStateAction (elastic#37870)
  Expose retention leases in shard stats (elastic#37991)
@jasontedor
Feedback
72e2e7b
@jasontedor
Copy link
Member Author

@elasticmachine run elasticsearch-ci/1

1 similar comment
@jasontedor
Copy link
Member Author

@elasticmachine run elasticsearch-ci/1

@jasontedor
Merge remote-tracking branch 'elastic/master' into replication-tracke…
65060ed 
…r-primary-term

* elastic/master:
  Mute failing date index name processor test
  Reenable BWC testing after retention lease stats (elastic#38062)
  Move watcher to use seq# and primary term for concurrency control (elastic#37977)
  ILM setPriority corrections for a 0 value (elastic#38001)
  Temporarily disable BWC for retention lease stats (elastic#38049)
  Skip Shrink when numberOfShards not changed (elastic#37953)
  Add dispatching to `HandledTransportAction` (elastic#38050)
  Update httpclient for JDK 11 TLS engine (elastic#37994)
  Reduce flaxiness of ccr recovery timeouts test (elastic#38035)
  Fix ILM status to allow unknown fields (elastic#38043)
  Fix ILM Lifecycle Policy to allow unknown fields (elastic#38041)
  Update verify repository to allow unknown fields (elastic#37619)
  [ML] Datafeed deprecation checks (elastic#38026)
  Deprecate minimum_master_nodes (elastic#37868)
@jasontedor jasontedor merged commit a9b12b3 into elastic:master Jan 31, 2019
jasontedor added a commit that referenced this pull request Jan 31, 2019
@jasontedor
Push primary term to replication tracker (#38044)
8073997 
This commit pushes the primary term into the replication tracker. This
is a precursor to using the primary term to resolving ordering problems
for retention leases. Namely, it can be that out-of-order retention
lease sync requests arrive on a replica. To resolve this, we need a
tuple of (primary term, version). For this to be, the primary term needs
to be accessible in the replication tracker. As the primary term is part
of the replication group anyway, this change conceptually makes sense.
@jasontedor jasontedor deleted the replication-tracker-primary-term branch January 31, 2019 14:36
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 31, 2019
@jasontedor
Merge branch 'master' into retention-leases-version
4aa6b77 
* master: (100 commits)
  Push primary term to replication tracker (elastic#38044)
  Introduce ability to minimize round-trips in CCS (elastic#37828)
  Don't Assert Ack on when Publish Timeout is 0 in Test (elastic#38077)
  Reduce object creation in Rounding class (elastic#38061)
  Treat put-mapping calls with `_doc` as a top-level key as typed calls. (elastic#38032)
  Fix test bug when testing the merging of mappings and templates. (elastic#38021)
  spelling: java script -- not JavaScript (elastic#37057)
  Enable SSL in reindex with security QA tests (elastic#37600)
  Disable BWC tests during backport (elastic#38074)
  SQL: Added SSL configuration options tests (elastic#37875)
  Minor fixes in the release notes script. (elastic#37967)
  Fix typo in docs. (elastic#38018)
  Update Lucene repo for 7.0.0-alpha2 (elastic#37985)
  Fix size of rolling-upgrade bootstrap config (elastic#38031)
  fix DateIndexNameProcessorTests offset pattern (elastic#38069)
  Speed up converting of temporal accessor to zoned date time (elastic#37915)
  Work around JDK8 timezone bug in tests (elastic#37968)
  Correct arg names when update mapping/settings from leader (elastic#38063)
  Introduce ssl settings to reindex from remote (elastic#37527)
  Mute testRetentionLeasesSyncOnExpiration
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnhatn dnhatn dnhatn left review comments

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
:Distributed/Engine Anything around managing Lucene and the Translog in an open shard. >refactoring v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasontedor @elasticmachine @ywelsch @dnhatn @colings86