[RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) #34902

tlrx · 2018-10-26T11:02:34Z

The current implementation of asyncBlockOperations() can be used to execute some code once all indexing operations permits have been acquired, then releases all permits immediately after the code execution. This immediate release is not suitable for treatments that need to keep all permits over multiple execution steps.

This commit adds a new asyncBlockOperations() that exposes a Releasable, making it possible to acquire all permits and only release them all when needed by closing the releasable.

This method is aimed to be used in a TransportReplicationAction that will acquire all permits on the primary shard.

The existing blockOperations() and asyncBlockOperations() methods have been modified to delegate permit acquisition/releasing to this new method.

Relates to #33888

elasticmachine · 2018-10-26T11:02:35Z

Pinging @elastic/es-distributed

bleskes

Looks great. I left some suggestions. I think this can go into master to reduce the maintenance overhead.

bleskes · 2018-10-26T11:26:51Z

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

@@ -123,23 +123,66 @@ public void close() {
     * @param onFailure the action to run if a failure occurs while blocking operations
     * @param <E>       the type of checked exception thrown by {@code onBlocked} (not thrown on the calling thread)
     */
-    <E extends Exception> void asyncBlockOperations(
-            final long timeout, final TimeUnit timeUnit, final CheckedRunnable<E> onBlocked, final Consumer<Exception> onFailure) {
+    <E extends Exception> void asyncBlockOperations(final long timeout, final TimeUnit timeUnit,


since we only have one user of this api, I wonder wether we should just remove this syntactic sugar method and put the single user to the other api

Right, let's do this.

bleskes · 2018-10-26T11:30:46Z

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

@@ -104,8 +104,8 @@ public void close() {
            final TimeUnit timeUnit,
            final CheckedRunnable<E> onBlocked) throws InterruptedException, TimeoutException, E {
        delayOperations();


I think we can fold delayOperations and releaseDelayedOperations into acquireAll which will simplify the code here and in asyncBlockOperations

In the case of asyncBlockOperations() we want to delay operations immediately and if we fold delayOperations() into acquireAll(), operations would start to be queued when the runnable is executed by the generic thread pool, not immediately - or am I missing something?

I missed that. Fair point.

tlrx · 2018-11-05T12:24:25Z

@bleskes I updated the code and removed the previous asyncBlockOperations so that there's only one async method now. I'm also OK to merge this into master directly, ~~I'll change the target branch once this is fully reviewed~~. Can you have another look please?

tlrx · 2018-11-06T15:51:22Z

Rebased on top of master to make CI happy.

bleskes

LGTM. Left some minor suggestions. No need for another run.

bleskes · 2018-11-07T09:09:16Z

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

            @Override
            public void onFailure(final Exception e) {
-                onFailure.accept(e);
+                try {
+                    onAcquired.onFailure(e);


This is interesting choice. I think I prefer to release delayed operations first, then call the onFailure handler. I don't think people can expect operations to continue being delayed and it's safer not to.

I've done this to keep the previous behavior of the asyncBlockOperations() where operations were released in the onAfter() method, so after the failure handler has been called.

But I share your opinion so I changed the onFailure logic to release operations first.

bleskes · 2018-11-07T09:15:36Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardOperationPermitsTests.java

-                e -> {
+            permits.asyncBlockOperations(new ActionListener<Releasable>() {
+                @Override
+                public void onResponse(final Releasable releasable) {


we use this pattern often - maybe introduce a simple class that wraps a runnable and runs it while releasing the releasable?

…ner<Releasable>) The current implementation of asyncBlockOperations() can be used to execute some code once all indexing operations permits have been acquired, then releases all permits immediately after the code execution. This immediate release is not suitable for treatments that need to keep all permits over multiple execution steps. This commit adds a new asyncBlockOperations() that exposes a Releasable, making it possible to acquire all permits and only release them all when needed by closing the Releasable. This method is aimed to be used in a TransportReplicationAction that will acquire all permits on the primary shard. The existing blockOperations() and asyncBlockOperations() methods have been modified to delegate permit acquisition/releasing to this new method. Relates to elastic#33888

s1monw

looks good left some suggestions

server/src/test/java/org/elasticsearch/index/shard/IndexShardOperationPermitsTests.java

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

tlrx · 2018-11-08T08:23:56Z

Thanks @bleskes @s1monw

* elastic/master: (25 commits) Fixes fast vector highlighter docs per issue 24318. (elastic#34190) [ML] Prevent notifications on deletion of a non existent job (elastic#35337) [CCR] Auto follow Coordinator fetch cluster state in system context (elastic#35120) Mute test for elastic#35361 Preserve `date_histogram` format when aggregating on unmapped fields (elastic#35254) Test: Mute failing SSL test Allow unmapped fields in composite aggregations (elastic#35331) [RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) (elastic#34902) HLRC: reindex API with wait_for_completion false (elastic#35202) Add docs on JNA temp directory not being noexec (elastic#35355) [CCR] Adjust list of dynamic index settings that should be replicated (elastic#35195) Replicate index settings to followers (elastic#35089) Rename RealmConfig.globalSettings() to settings() (elastic#35330) [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329) Scripting: Add back lookup vars in score script (elastic#34833) watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271) Remove ALL shard check in CheckShrinkReadyStep (elastic#35346) Use soft-deleted docs to resolve strategy for engine operation (elastic#35230) [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316) Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160) ...

pcsanwald · 2018-11-09T12:25:51Z

@tlrx I added v7.0.0 label here, please remove if that seems wrong.

tlrx · 2018-11-09T13:30:55Z

Thanks @pcsanwald and sorry - it was intended to be merged in a feature branch and then we decided that it could go in master but I forgot to add the version label.

…ner<Releasable>) (elastic#34902) The current implementation of asyncBlockOperations() can be used to execute some code once all indexing operations permits have been acquired, then releases all permits immediately after the code execution. This immediate release is not suitable for treatments that need to keep all permits over multiple execution steps. This commit adds a new asyncBlockOperations() that exposes a Releasable, making it possible to acquire all permits and only release them all when needed by closing the Releasable. The existing blockOperations() method has been modified to delegate permit acquisition/releasing to this new method. Relates to elastic#33888

…ner<Releasable>) (#34902) The current implementation of asyncBlockOperations() can be used to execute some code once all indexing operations permits have been acquired, then releases all permits immediately after the code execution. This immediate release is not suitable for treatments that need to keep all permits over multiple execution steps. This commit adds a new asyncBlockOperations() that exposes a Releasable, making it possible to acquire all permits and only release them all when needed by closing the Releasable. The existing blockOperations() method has been modified to delegate permit acquisition/releasing to this new method. Relates to #33888

tlrx added >enhancement :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Oct 26, 2018

bleskes suggested changes Oct 26, 2018

View reviewed changes

tlrx changed the base branch from replicated-closed-indices to master November 5, 2018 12:20

tlrx changed the base branch from master to replicated-closed-indices November 5, 2018 12:21

tlrx changed the base branch from replicated-closed-indices to master November 6, 2018 15:49

tlrx force-pushed the indexing-ops-permits-with-releasable branch from 2554506 to 9b456e0 Compare November 6, 2018 15:50

tlrx mentioned this pull request Nov 7, 2018

Replicate closed indices #33888

Closed

50 tasks

bleskes approved these changes Nov 7, 2018

View reviewed changes

tlrx force-pushed the indexing-ops-permits-with-releasable branch from dcc8abe to 16e2967 Compare November 7, 2018 12:02

tlrx added 3 commits November 7, 2018 13:50

Apply feedback

b15647b

Apply feedback

ef1a506

tlrx force-pushed the indexing-ops-permits-with-releasable branch from 16e2967 to ef1a506 Compare November 7, 2018 12:50

s1monw reviewed Nov 7, 2018

View reviewed changes

server/src/test/java/org/elasticsearch/index/shard/IndexShardOperationPermitsTests.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/shard/IndexShardOperationPermits.java Show resolved Hide resolved

throw new AssertionError(e);

338b6ae

tlrx merged commit 1703a61 into elastic:master Nov 8, 2018

tlrx deleted the indexing-ops-permits-with-releasable branch November 8, 2018 08:23

tlrx mentioned this pull request Nov 8, 2018

[RCI] Check blocks while having index shard permit in TransportReplicationAction #35332

Merged

pcsanwald added the v7.0.0 label Nov 9, 2018

tlrx mentioned this pull request Nov 13, 2018

Add RunOnce utility class that executes a Runnable exactly once #35484

Closed

tlrx added the v6.6.0 label Nov 14, 2018

DaveCTurner mentioned this pull request Nov 15, 2018

Rolling-upgrade RecoveryIT tests are broken #35597

Closed

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

[RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) #34902

[RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) #34902

tlrx commented Oct 26, 2018 •

edited

Loading

elasticmachine commented Oct 26, 2018

bleskes left a comment

bleskes Oct 26, 2018

tlrx Nov 5, 2018

bleskes Oct 26, 2018

tlrx Oct 26, 2018

bleskes Nov 5, 2018

tlrx commented Nov 5, 2018 •

edited

Loading

tlrx commented Nov 6, 2018

bleskes left a comment

bleskes Nov 7, 2018

tlrx Nov 7, 2018

bleskes Nov 7, 2018

tlrx Nov 7, 2018

s1monw left a comment

tlrx commented Nov 8, 2018

pcsanwald commented Nov 9, 2018

tlrx commented Nov 9, 2018

[RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) #34902

[RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) #34902

Conversation

tlrx commented Oct 26, 2018 • edited Loading

elasticmachine commented Oct 26, 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

tlrx commented Nov 5, 2018 • edited Loading

tlrx commented Nov 6, 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

s1monw left a comment

Choose a reason for hiding this comment

tlrx commented Nov 8, 2018

pcsanwald commented Nov 9, 2018

tlrx commented Nov 9, 2018

tlrx commented Oct 26, 2018 •

edited

Loading

tlrx commented Nov 5, 2018 •

edited

Loading