Expose all permits acquisition in IndexShard and TransportReplicationAction #35540

tlrx · 2018-11-14T12:54:49Z

This pull request exposes two new methods in the IndexShard and TransportReplicationAction classes in order to allow transport replication actions to acquire all index shard operation permits for their execution.

It first adds the acquirePrimaryAllOperationsPermits() and the acquireReplicaAllOperationsPermits() methods to the IndexShard class which allow to acquire all operations permits on a shard while exposing a Releasable. It also refactors the TransportReplicationAction class to expose two protected methods (acquirePrimaryOperationPermit() and acquireReplicaOperationPermit()) that can be overridden when a transport replication action requires the acquisition of all permits on primary and/or replica shard during execution.

Finally, it adds a TransportReplicationAllPermitsAcquisitionTests which illustrates how a transport replication action can grab all permits before adding a cluster block in the cluster state, making subsequent operations that requires a single permit to fail (such test has been discussed in #35332 (comment)).

Related to elastic #33888

elasticmachine · 2018-11-14T12:54:51Z

Pinging @elastic/es-distributed

bleskes

Thx @tlrx. I left initial feedback on the production call.

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

bleskes · 2018-11-14T13:46:34Z

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

@@ -697,7 +728,7 @@ public void onFailure(Exception e) {
        }
    }

-    protected IndexShard getIndexShard(ShardId shardId) {
+    protected IndexShard getIndexShard(final ShardId shardId, final String targetAllocationID) {


why is the targetAllocaitonID passed here? I guess it should be validated, but it isn't?

This is a leftover that was helpful in TransportReplicationAllPermitsAcquisitionTests to dinstinguish between the primary and replica shard when executing a transport replication test action.

I reverted this and changed how the action in test retrieves the index shard.

bleskes · 2018-11-14T13:48:35Z

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

+     * Acquire all primary operation permits. Once all permits are acquired, the provided ActionListener is called.
+     * It is the responsibility of the caller to close the {@link Releasable}.
+     */
+    public void acquirePrimaryAllOperationsPermits(final ActionListener<Releasable> onPermitAcquired, final TimeValue timeout) {


nit: acquireAllPrimaryOperationPermits?

bleskes · 2018-11-14T15:26:10Z

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

+     * an {@link IllegalStateException}. Otherwise the global checkpoint and the max_seq_no_of_updates marker of the replica are updated
+     * before the invocation of the {@link ActionListener#onResponse(Object)}} method of the provided listener.
+     */
+    private ActionListener<Releasable> createListener(final ActionListener<Releasable> listener,


I'm not so comfortable with separating this code from the one in updatePrimaryTermIfNeeded - they are tightly connected. Instead of sharing code this way, how about creating a callback that will run:

indexShardOperationPermits.acquire(listener, executorOnDelay, true, debugInfo);

or

indexShardOperationPermits.asyncBlockOperations(listener, timeout.duration(), timeout.timeUnit());

I share your concerns and I updated the code, let me know what you think.

tlrx · 2018-11-15T13:11:05Z

Thanks @bleskes, I addressed your first bunch of comments. I also added comments to the TransportReplicationAllPermitsAcquisitionTests and reworked it a bit so that it is easier to understand.

bleskes

LGTM. I left one suggestion for a potential follow up PR.

bleskes · 2018-11-15T15:17:29Z

...elasticsearch/action/support/replication/TransportReplicationAllPermitsAcquisitionTests.java

+                allPermitsAction.new AsyncPrimaryAction(request(), allocationId(), primaryTerm(), transportChannel(allPermitFuture), null) {
+                    @Override
+                    void runWithPrimaryShardReference(final TransportReplicationAction.PrimaryShardReference reference) {
+                        assertEquals("All permits must be acquired", 0, reference.indexShard.getActiveOperationsCount());


This is indicates a funky property of getActiveOperationsCount. To date all the blocks were internal to the shard and thus 0 was a good return. I don't really have a good solution in mind. At the very least we should document this on getActiveOperationsCount. Maybe 1 (we know there is only one exclusive op) is a better return value? /cc @ywelsch

I take the point, thanks.

sorry, I missed the ping here. I think that 1 is ok too, but then we should in general change the terminology in IndexShardOperationPermits to move from the notion of blocking operations to running exclusive operations (similar to a read/write lock).

bleskes · 2018-11-15T15:51:51Z

...elasticsearch/action/support/replication/TransportReplicationAllPermitsAcquisitionTests.java

+            if (this.shardId.equals(shardId) == false) {
+                throw new AssertionError("shard id differs from " + shardId);
+            }
+            return (executedOnPrimary.get() == null) ? primary : replica;


I know, not really proud of this... I'll take a look at how to improve this test.

…cationAction This commit adds the acquirePrimaryAllOperationsPermits() and the acquireReplicaAllOperationsPermits() methods to the IndexShard class. These methods allow to acquire all operations permits on a primary or a replica shard and can be used in future transport replication actions to acquire all permits instead of a single one. Related to elastic elastic#33888

…Action (#35540) This pull request exposes two new methods in the IndexShard and TransportReplicationAction classes in order to allow transport replication actions to acquire all index shard operation permits for their execution. It first adds the acquireAllPrimaryOperationPermits() and the acquireAllReplicaOperationsPermits() methods to the IndexShard class which allow to acquire all operations permits on a shard while exposing a Releasable. It also refactors the TransportReplicationAction class to expose two protected methods (acquirePrimaryOperationPermit() and acquireReplicaOperationPermit()) that can be overridden when a transport replication action requires the acquisition of all permits on primary and/or replica shard during execution. Finally, it adds a TransportReplicationAllPermitsAcquisitionTests which illustrates how a transport replication action can grab all permits before adding a cluster block in the cluster state, making subsequent operations that requires a single permit to fail). Related to elastic #33888

tlrx added >enhancement v7.0.0 :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v6.6.0 labels Nov 14, 2018

tlrx requested review from s1monw, bleskes and ywelsch November 14, 2018 12:54

bleskes suggested changes Nov 14, 2018

View reviewed changes

bleskes approved these changes Nov 15, 2018

View reviewed changes

tlrx added 4 commits November 22, 2018 12:20

Get rid of onReferenceAcquired

c43177e

Rename methods

493cd16

Apply feedback

ef255a2

tlrx force-pushed the add-indexshard-acquire-all-permits branch from 4865718 to ef255a2 Compare November 22, 2018 12:11

tlrx merged commit 2e37f17 into elastic:master Nov 23, 2018

tlrx deleted the add-indexshard-acquire-all-permits branch November 23, 2018 08:27

This was referenced Nov 23, 2018

[Tests] Do not use all permits acquisition in replica promotion tests #35862

Closed

Allow asynchronous block operations to be delayed in IndexShardOperationPermits #35999

Closed

tlrx mentioned this pull request Dec 5, 2018

Replicate closed indices #33888

Closed

50 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose all permits acquisition in IndexShard and TransportReplicationAction #35540

Expose all permits acquisition in IndexShard and TransportReplicationAction #35540

tlrx commented Nov 14, 2018

elasticmachine commented Nov 14, 2018

bleskes left a comment

bleskes Nov 14, 2018

tlrx Nov 15, 2018

bleskes Nov 14, 2018

tlrx Nov 15, 2018

bleskes Nov 14, 2018

tlrx Nov 15, 2018

tlrx commented Nov 15, 2018

bleskes left a comment

bleskes Nov 15, 2018

tlrx Nov 23, 2018

ywelsch Nov 23, 2018

bleskes Nov 23, 2018

bleskes Nov 15, 2018

tlrx Nov 23, 2018

Expose all permits acquisition in IndexShard and TransportReplicationAction #35540

Expose all permits acquisition in IndexShard and TransportReplicationAction #35540

Conversation

tlrx commented Nov 14, 2018

elasticmachine commented Nov 14, 2018

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Nov 15, 2018

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment