Make Transport Shard Bulk Action Async #39793

original-brownbear · 2019-03-07T15:12:04Z

This is a dependency of #39504

Motivation:
By refactoring TransportShardBulkAction#shardOperationOnPrimary to async, we enable using DeterministicTaskQueue based tests to run indexing operations. This was previously impossible since we were blocking on the write thread until the update thread finished the mapping update.
With this change, the mapping update will trigger a new task in the write queue instead.
This change significantly enhances the amount of coverage we get from SnapshotResiliencyTests (and other potential future tests) when it comes to tracking down concurrency issues with distributed state machines.

The logical change is effectively all in TransportShardBulkAction, the rest of the changes is then simply mechanically moving the caller code and tests to being async and passing the ActionListener down.

Since the move to async would've added more parameters to the private static steps in this logic, I decided to inline and dry up (between delete and update) the logic as much as I could instead of passing the listener + wait-consumer down through all of them.

* Dependency of elastic#39504

elasticmachine · 2019-03-07T15:12:05Z

Pinging @elastic/es-distributed

original-brownbear · 2019-03-07T15:15:13Z

server/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java

+            protected void doRun() {
+                while (context.hasMoreOperationsToExecute()) {
+                    if (executeBulkItemRequest(context, updateHelper, nowInMillisSupplier, mappingUpdater, waitForMappingUpdate,
+                        ActionListener.wrap(v -> executor.execute(this), listener::onFailure)) == false) {


We are moving from the update to the write thread here after a mapping update updates the cluster state. I'm a little unsure about this tbh. Is it a problem that when we're under pressure and could get a bulk rejection that the mapping update could have happened but the write then got rejected?
I assumed not, as the same kind of inconsistency can happen if the ES process dies after the mapping update in the current implementation, but thought I should point that out (obviously this is a much more likely spot now when bulk rejections happen).

The main reason an earlier refactoring of this class did not move this bit to async code was that this part here is problematic. We might have partially executed the batch and generated sequence numbers for it. Failing the request here now is not an option as it will otherwise have the replica go out of sync with the primary, lead to gaps in the history on the replica and block local and global checkpoint advancement, given that the partially executed request won't be replicated.
There are two options to explore: 1) forcing this onto the write executor or 2) marking the current and all subsequent bulk items as failed, and then replicate the request normally to the replicas. I would be interested in @bleskes's thoughts on this.

Also this will require extensive testing given the brittle nature of it.

Indeed we avoided doing this in order to not have to deal with rejections / not to have a way bypass queue sizes (which will happen with force execution) at the time. I think the added value of the extra testing coverage merits exploring our options here. I'm inclined to go with the second suggestion (mark the rest of the operations as failed on rejection). I'm not too worried about having committed the mapping changes but ending up not using them. I think that just an edge case and as Armin mentioned - it can happen anyway.

@ywelsch @bleskes thanks for taking a look. I implemented option 2 real quick in f3b59c2 (on bulk reject just loop through all remaining requests and fail them). If this looks ok, I'd look into adding test coverage for the scenario mentioned above?

I think I was also able to write a valid test (at least I reproduced the issue that @ywelsch mentioned) in a3c8cac. Take a look when you have a sec :)

server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java

original-brownbear · 2019-03-07T15:39:07Z

Jenkins run elasticsearch-ci/packaging-sample (vagrant timed out)

original-brownbear · 2019-03-07T18:37:56Z

Jenkins run elasticsearch-ci/packaging-sample (vagrant timed out again)

original-brownbear · 2019-03-17T16:18:14Z

@ywelsch @dnhatn any chance one of you could take a look here? This one isn't super urgent but it's blocking further progress on fixing snapshot repository stability since it holds up #39504 so it would be great if we could continue here next week or so :)

original-brownbear · 2019-03-23T05:45:05Z

Jenkins run elasticsearch-ci/2 (Vbox failed to come up)

* Fixing minor mistake from #39793 here, we should be using `run` so that the `onFailure` path is executed if the first invocation of this `Runnable` fails for an unexpected reason

* elastic/master: Fix Failing to Handle Ex. in TransportShardBulkAction (elastic#40923) Be lenient when parsing build flavor and type on the wire (elastic#40734) Make Transport Shard Bulk Action Async (elastic#39793)

* Thanks to elastic#39793 dynamic mapping updates don't contain blocking operations anymore so we don't have to manually put the mapping in this test and can keep it a little simpler

* In elastic#39793 this assertion was added under the assumption that no exceptions would be thrown in this method, which turned out not to be correct and at the very least `org.elasticsearch.index.shard.IndexShardClosedException` can be thrown by `org.elasticsearch.index.shard.IndexShard.sync` * Closes elastic#40933

* Remove Overly Strict Assertion in TransportShardBulkAction * In #39793 this assertion was added under the assumption that no exceptions would be thrown in this method, which turned out not to be correct and at the very least `org.elasticsearch.index.shard.IndexShardClosedException` can be thrown by `org.elasticsearch.index.shard.IndexShard.sync` * Closes #40933

* Prior to elastic#39793 exceptions for the primary write and delete actions were bubbled up to the caller so that closed shards would be handled accordingly upstream. elastic#39793 accidentally changed the behaviour here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards. * This fix returns to that behaviour and adjusts the listeners passed in `TransportReplicationAction` such that they behave like the previous synchronous `catch`. * Dried up the exception handling slightly for that and inlined all the listeners to make the logic a little easier to follow * Reenable SplitIndexIT now that clsoed shards are properly handled again * Closes elastic#40944

* Prior to #39793 exceptions for the primary write and delete actions were bubbled up to the caller so that closed shards would be handled accordingly upstream. #39793 accidentally changed the behaviour here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards. * This fix returns to that behaviour and adjusts the listeners passed in `TransportReplicationAction` such that they behave like the previous synchronous `catch`. * Dried up the exception handling slightly for that and inlined all the listeners to make the logic a little easier to follow * Reenable SplitIndexIT now that clsoed shards are properly handled again * Closes #40944

This is a dependency of elastic#39504 Motivation: By refactoring `TransportShardBulkAction#shardOperationOnPrimary` to async, we enable using `DeterministicTaskQueue` based tests to run indexing operations. This was previously impossible since we were blocking on the `write` thread until the `update` thread finished the mapping update. With this change, the mapping update will trigger a new task in the `write` queue instead. This change significantly enhances the amount of coverage we get from `SnapshotResiliencyTests` (and other potential future tests) when it comes to tracking down concurrency issues with distributed state machines. The logical change is effectively all in `TransportShardBulkAction`, the rest of the changes is then simply mechanically moving the caller code and tests to being async and passing the `ActionListener` down. Since the move to async would've added more parameters to the `private static` steps in this logic, I decided to inline and dry up (between delete and update) the logic as much as I could instead of passing the listener + wait-consumer down through all of them.

This is a dependency of #39504 Motivation: By refactoring `TransportShardBulkAction#shardOperationOnPrimary` to async, we enable using `DeterministicTaskQueue` based tests to run indexing operations. This was previously impossible since we were blocking on the `write` thread until the `update` thread finished the mapping update. With this change, the mapping update will trigger a new task in the `write` queue instead. This change significantly enhances the amount of coverage we get from `SnapshotResiliencyTests` (and other potential future tests) when it comes to tracking down concurrency issues with distributed state machines. The logical change is effectively all in `TransportShardBulkAction`, the rest of the changes is then simply mechanically moving the caller code and tests to being async and passing the `ActionListener` down. Since the move to async would've added more parameters to the `private static` steps in this logic, I decided to inline and dry up (between delete and update) the logic as much as I could instead of passing the listener + wait-consumer down through all of them.

* Thanks to #39793 dynamic mapping updates don't contain blocking operations anymore so we don't have to manually put the mapping in this test and can keep it a little simpler

* Thanks to elastic#39793 dynamic mapping updates don't contain blocking operations anymore so we don't have to manually put the mapping in this test and can keep it a little simpler

* Thanks to #39793 dynamic mapping updates don't contain blocking operations anymore so we don't have to manually put the mapping in this test and can keep it a little simpler

This is a dependency of elastic#39504 Motivation: By refactoring `TransportShardBulkAction#shardOperationOnPrimary` to async, we enable using `DeterministicTaskQueue` based tests to run indexing operations. This was previously impossible since we were blocking on the `write` thread until the `update` thread finished the mapping update. With this change, the mapping update will trigger a new task in the `write` queue instead. This change significantly enhances the amount of coverage we get from `SnapshotResiliencyTests` (and other potential future tests) when it comes to tracking down concurrency issues with distributed state machines. The logical change is effectively all in `TransportShardBulkAction`, the rest of the changes is then simply mechanically moving the caller code and tests to being async and passing the `ActionListener` down. Since the move to async would've added more parameters to the `private static` steps in this logic, I decided to inline and dry up (between delete and update) the logic as much as I could instead of passing the listener + wait-consumer down through all of them.

* Fixing minor mistake from elastic#39793 here, we should be using `run` so that the `onFailure` path is executed if the first invocation of this `Runnable` fails for an unexpected reason

) * Remove Overly Strict Assertion in TransportShardBulkAction * In elastic#39793 this assertion was added under the assumption that no exceptions would be thrown in this method, which turned out not to be correct and at the very least `org.elasticsearch.index.shard.IndexShardClosedException` can be thrown by `org.elasticsearch.index.shard.IndexShard.sync` * Closes elastic#40933

* Prior to elastic#39793 exceptions for the primary write and delete actions were bubbled up to the caller so that closed shards would be handled accordingly upstream. elastic#39793 accidentally changed the behaviour here and simply marked those exceptions as bulk item failures on the request and kept processing bulk request items on closed shards. * This fix returns to that behaviour and adjusts the listeners passed in `TransportReplicationAction` such that they behave like the previous synchronous `catch`. * Dried up the exception handling slightly for that and inlined all the listeners to make the logic a little easier to follow * Reenable SplitIndexIT now that clsoed shards are properly handled again * Closes elastic#40944

* Thanks to elastic#39793 dynamic mapping updates don't contain blocking operations anymore so we don't have to manually put the mapping in this test and can keep it a little simpler

Make TransportBulkAction Non-Blocking

5ee4211

* Dependency of elastic#39504

original-brownbear added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >refactoring v8.0.0 v7.2.0 labels Mar 7, 2019

original-brownbear commented Mar 7, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/action/index/MappingUpdatedAction.java Outdated Show resolved Hide resolved

original-brownbear requested review from dnhatn and ywelsch March 7, 2019 19:58

original-brownbear added 8 commits March 11, 2019 21:51

Merge remote-tracking branch 'elastic/master' into async-bulk-action

788efd3

Merge remote-tracking branch 'elastic/master' into async-bulk-action

36508a6

Merge remote-tracking branch 'elastic/master' into async-bulk-action

40a3c8d

shorter diff

764b70b

shorter

6a59fa0

shorter

0bb09ee

fix duplication

9a22a36

fix test

8df5f96

original-brownbear added 2 commits March 22, 2019 19:19

Merge remote-tracking branch 'elastic/master' into async-bulk-action

b47e5f7

CR: handle bulk rejections after mapping updates

f3b59c2

original-brownbear added 2 commits March 24, 2019 16:24

Merge remote-tracking branch 'elastic/master' into async-bulk-action

0b3b4ab

CR: add test for bulk rejection behaviour

a3c8cac

original-brownbear requested a review from bleskes March 24, 2019 20:13

Merge remote-tracking branch 'elastic/master' into async-bulk-action

7f8700a

original-brownbear mentioned this pull request Mar 29, 2019

Add Restore Operation to SnapshotResiliencyTests #40634

Merged

Merge remote-tracking branch 'elastic/master' into async-bulk-action

5780183

original-brownbear mentioned this pull request Apr 7, 2019

Simplify Snapshot Resiliency Test #40930

Merged

dnhatn mentioned this pull request Apr 8, 2019

AnnotationIndexIT.testCreatedWhenAfterOtherMlIndex fails #40933

Closed

original-brownbear mentioned this pull request Apr 8, 2019

Relax Overly Strict Assertion in TransportShardBulkAction #40940

Merged

original-brownbear mentioned this pull request Apr 8, 2019

SplitIndexIT fails with AlreadyClosedException #40944

Closed

original-brownbear mentioned this pull request Apr 9, 2019

Fix Exception Handling for TransportShardBulkAction #41006

Merged

original-brownbear removed the backport pending label Apr 11, 2019

original-brownbear mentioned this pull request Apr 11, 2019

Make Transport Shard Bulk Action Async (#39793) #41112

Merged

original-brownbear mentioned this pull request Apr 26, 2019

Simplify Snapshot Resiliency Test (#40930) #41565

Merged

pangyikhei mentioned this pull request Aug 6, 2019

Scripting: ctx._now is different when using Bulk API compared to Update API #45254

Closed

DaveCTurner mentioned this pull request Jan 6, 2020

Dynamic mapping updates are unboundedly parallel #50670

Closed

ywelsch mentioned this pull request Feb 5, 2020

Elasticsearch is fsyncing on transport threads #51904

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Transport Shard Bulk Action Async #39793

Make Transport Shard Bulk Action Async #39793

original-brownbear commented Mar 7, 2019 •

edited

Loading

elasticmachine commented Mar 7, 2019

original-brownbear Mar 7, 2019

ywelsch Mar 22, 2019

bleskes Mar 22, 2019

original-brownbear Mar 22, 2019

original-brownbear Mar 24, 2019

original-brownbear commented Mar 7, 2019

original-brownbear commented Mar 7, 2019

original-brownbear commented Mar 17, 2019

original-brownbear commented Mar 23, 2019

Make Transport Shard Bulk Action Async #39793

Make Transport Shard Bulk Action Async #39793

Conversation

original-brownbear commented Mar 7, 2019 • edited Loading

elasticmachine commented Mar 7, 2019

original-brownbear Mar 7, 2019

Choose a reason for hiding this comment

ywelsch Mar 22, 2019

Choose a reason for hiding this comment

bleskes Mar 22, 2019

Choose a reason for hiding this comment

original-brownbear Mar 22, 2019

Choose a reason for hiding this comment

original-brownbear Mar 24, 2019

Choose a reason for hiding this comment

original-brownbear commented Mar 7, 2019

original-brownbear commented Mar 7, 2019

original-brownbear commented Mar 17, 2019

original-brownbear commented Mar 23, 2019

original-brownbear commented Mar 7, 2019 •

edited

Loading