Port Primary Terms to master #17044

bleskes · 2016-03-10T10:24:37Z

Primary terms is a way to make sure that operations replicated from stale primary are rejected by shards following a newly elected primary.

Original PRs adding this to the seq# feature branch #14062 , #14651

Relates to #17038

jasontedor · 2016-03-10T22:32:25Z

core/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -447,6 +446,9 @@ private IndexShardState changeState(IndexShardState newState, String reason) {

    public Engine.Index prepareIndexOnReplica(SourceToParse source, long version, VersionType versionType) {
        try {
+            if (shardRouting.primary() && shardRouting.isRelocationTarget() == false) {
+                throw new IllegalIndexShardStateException(shardId, state, "shard is not a replica");


This exception will be treated as ignore replica exception. 😉

I think this check is wrong. When we have relocation going on and relocation source is marked as relocated (i.e. we call executeRemotely in TransportReplicationAction), then we have primary relocation target replicating back to primary relocation source (see also ReplicationPhase).

bleskes · 2016-03-11T07:49:10Z

@jasontedor pushed an update to those exceptions... sadly testing is harder (but will be possible with new testing infra I'm working on...)

ywelsch · 2016-03-11T07:50:32Z

core/src/main/java/org/elasticsearch/action/support/replication/ReplicationRequest.java

+    /** marks the primary term in which the operation was performed */
+    public void primaryTerm(long term) {
+        primaryTerm = term;
+    }


I guess this can be reused to replace routedBasedOnClusterVersion (#16274). Yay 👍

interesting idea - it's currently not incremented when relocating a primary though... requires more thought.

ywelsch · 2016-03-11T09:10:37Z

Thanks @bleskes for porting this to master. Primary terms will be really helpful in many kind of resiliency-related scenarios (solved previously with ugly hacks, e.g. routedBasedOnClusterVersion (#16274)).
I left some comments and more general thoughts about primary terms. I haven't looked at src/test/** yet but can do this in a second iteration.

bleskes · 2016-03-12T10:58:04Z

@ywelsch I pushed a new implementation. I'm starting to warm up to making shard routing immutable on the index shard level (different PR, and not now :))

bleskes · 2016-03-13T08:00:06Z

Pushed another commit fixing a side effect of changing the exception types from IllegalShardStateException to IllegalArgumentException (to help with #17038 ) - the replica wrapper shouldn't fail shards locally anymore (technical engine level errors are still dealt with by the engine), but leave it to the primary. It was originally added in #5847 but with current machinery it's not needed.

It's also useful to note that adding assertions of the terms invariants to IndexShard forced some clean ups in IndicesClusterService (as @ywelsch already suspected).

I will update the PR description with these and whatever else we find once the review cycles are done.

ywelsch · 2016-03-14T08:53:53Z

core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java

@@ -1128,6 +1106,9 @@ protected boolean shouldExecuteReplication(Settings settings) {
        boolean isRelocated();
        void failShard(String reason, @Nullable Throwable e);
        ShardRouting routingEntry();
+
+        /** returns the primary term of the current opration */


opration -> operation

ywelsch · 2016-03-14T10:42:44Z

core/src/main/java/org/elasticsearch/indices/cluster/IndicesClusterStateService.java

                        }
-                        indexService.removeShard(existingShardId, "removing shard (index is closed)");
+                        indexService.removeShard(existingShard.id(), "removing shard (index is closed)");


Is this somewhat duplicate functionality that is also in applyCleanedIndices? Maybe combine both of them here? It is weird that applyCleanedIndices removes shards.

agreed and discussed :)

ywelsch · 2016-03-14T11:45:40Z

Left comments/questions here and there. Updating primary terms in AllocationService makes it so much easier to understand how they change (just a single if statement, yay) 😄.

bleskes · 2016-03-14T20:06:51Z

@ywelsch thanks for the review. I pushed an update

ywelsch · 2016-03-24T13:58:50Z

core/src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java

+                        // we do not use newPrimary.isTargetRelocationOf(oldPrimary) because that one enforces newPrimary to
+                        // be initializing. However, when the target shard is activated, we still want the primary term to staty
+                        // the same
+                        (oldPrimary.relocating() && newPrimary.isSameAllocation(oldPrimary.buildTargetRelocatingShard()))) {


I believe we can safely expand the semantics of isRelocationTargetOf to also incorporate this case. This means changing isRelocationTarget, isRelocationTargetOf and isRelocationSourceOf such that it not only accounts for INITIALIZING state. (It is a safe change as a STARTED shard can never be a relocation source and RELOCATING shard can never be a relocation target).

For example, replacing

public boolean isRelocationTargetOf(ShardRouting other) { return this.allocationId != null && other.allocationId != null && this.state == ShardRoutingState.INITIALIZING && this.allocationId.getId().equals(other.allocationId.getRelocationId());

by

public boolean isRelocationTargetOf(ShardRouting other) { return this.allocationId != null && other.allocationId != null && other.state == ShardRoutingState.RELOCATING && this.allocationId.getId().equals(other.allocationId.getRelocationId());

Can be a follow-up, just something to think about.

I agree - this is confusing ( I got it wrong first too). I don't want to expand the scope of this PR too much , so prefer a follow up..

ywelsch · 2016-03-24T22:07:46Z

Left really minor comments. I think we're good with the changes in IndicesClusterStateService. Thanks again for porting this to master.

bleskes · 2016-03-25T10:19:33Z

Thx @ywelsch @jasontedor . I pushed an update and responded to comments

ywelsch · 2016-03-25T10:29:09Z

LGTM

Primary terms is a way to make sure that operations replicated from stale primary are rejected by shards following a newly elected primary. Original PRs adding this to the seq# feature branch elastic#14062 , elastic#14651 . Unlike those PR, here we take a different approach (based on newer code in master) where the primary terms are stored in the meta data only (and not in `ShardRouting` objects). Relates to elastic#17038 Closes elastic#17044

…ica should be retried In extreme cases a local primary shard can be replaced with a replica while a replication request is in flight and the primary action is applied to the shard (via `acquirePrimaryOperationLock()). elastic#17044 changed the exception used in that method to something that isn't recognized as `TransportActions.isShardNotAvailableException`, causing the operation to fail immediately instead of retrying. This commit fixes this by check the primary flag before acquiring the lock. This is safe to do as an IndexShard will never be demoted once a primary. Closes elastic#17358

bleskes added >enhancement resiliency :Core/Infra/Core Core issues without another label v5.0.0-alpha1 labels Mar 10, 2016

bleskes assigned jasontedor Mar 10, 2016

jasontedor reviewed Mar 10, 2016
View reviewed changes

ywelsch reviewed Mar 11, 2016
View reviewed changes

ywelsch reviewed Mar 14, 2016
View reviewed changes

ywelsch reviewed Mar 24, 2016
View reviewed changes

bleskes force-pushed the primary_terms branch from 373ef71 to cbfe689 Compare March 25, 2016 10:31

bleskes force-pushed the primary_terms branch from 301f855 to fe43eef Compare March 25, 2016 11:11

bleskes merged commit fe43eef into elastic:master Mar 25, 2016

bleskes deleted the primary_terms branch March 25, 2016 11:16

bleskes mentioned this pull request Mar 30, 2016

Replication operation that try to perform the primary phase on a replica should be retried #17358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Primary Terms to master #17044

Port Primary Terms to master #17044

bleskes commented Mar 10, 2016

jasontedor Mar 10, 2016

ywelsch Mar 11, 2016

bleskes commented Mar 11, 2016

ywelsch Mar 11, 2016

bleskes Mar 11, 2016

ywelsch commented Mar 11, 2016

bleskes commented Mar 12, 2016

bleskes commented Mar 13, 2016

ywelsch Mar 14, 2016

ywelsch Mar 14, 2016

bleskes Mar 14, 2016

ywelsch commented Mar 14, 2016

bleskes commented Mar 14, 2016

ywelsch Mar 24, 2016

bleskes Mar 24, 2016

ywelsch commented Mar 24, 2016

bleskes commented Mar 25, 2016

ywelsch commented Mar 25, 2016

Port Primary Terms to master #17044

Port Primary Terms to master #17044

Conversation

bleskes commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Mar 11, 2016

bleskes commented Mar 12, 2016

bleskes commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Mar 14, 2016

bleskes commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Mar 24, 2016

bleskes commented Mar 25, 2016

ywelsch commented Mar 25, 2016