Implement incremental bulk execution #113044

Tim-Brooks · 2024-09-17T18:06:01Z

This commit adds a mechanism of splitting bulk requests when a
coordinating nodes is under memory pressure. In order to do that it
adds a new mechanism of http stream handling. And the proper framework
to use this with bulk requests.

elasticsearchmachine · 2024-09-17T18:06:24Z

Pinging @elastic/es-distributed (Team:Distributed)

nielsbauman

The changes in BulkOperation LGTM. I do think it would be worth adding one test that ensures that short-circuited shard requests also end up in the failure store - maybe in IncrementalBulkRestIT.java or IncrementalBulkIT.java perhaps.

henningandersen

LGTM.

henningandersen · 2024-09-18T12:03:00Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java


+            if (request.isStreamedContent()) {
+                assert action instanceof RequestBodyChunkConsumer;
+                var chunkConsumer = (RequestBodyChunkConsumer) action;


I think this is where we'd fail if there is a discrepancy between the handlers and the decider filter in Netty4HttpAggregator. This is fine for now (for the merge), and possibly also for our initial roll-out plans.

I do wonder though if we could have a simple aggregating handler here that protects us against a discrepancy, in particular before we enable this for regular releases (or perhaps before that even)? Would it be feasible to always do the aggregation here even (when the mode is enabled), assuming larger requests are handled by proper handlers?

It's a temporary measure to do minimal change in http/rest layer to allow partial content. Current intention is to remove HttpAggregator and do aggregation in (Base)RestHandler. I drafted it before in #112120 and planning to revisit once we merge to main. There are several options how to do that, I will prepare a design doc.

henningandersen · 2024-09-18T12:08:18Z

modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpAggregator.java

+    public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
+        assert msg instanceof HttpObject;
+        if (msg instanceof HttpRequest request) {
+            var preReq = HttpHeadersAuthenticatorUtils.asHttpPreRequest(request);


I find this somewhat unnecessary, could the decider not work on the uri directly instead or even the HttpRequest (the decider is quite intimate anyway). Happy to leave as is for now ofc.

Yes. IMO we definitely have room for improvement here. There is a ticket tracking improved handling in this area.

henningandersen · 2024-09-18T12:10:24Z

I do think it would be worth adding one test that ensures that short-circuited shard requests also end up in the failure store - maybe in IncrementalBulkRestIT.java or IncrementalBulkIT.java perhaps.

Thanks, that makes sense. We should tackle that as a follow-up though, given that the incremental bulk handling is off by default and I'd prefer to get this in and start some early testing on it.

jbaiera

BulkOperation changes look like they make sense to me! I second the ask for a test, but it's fine if that comes as a follow up.

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

Allow a single bulk request to be passed to Elasticsearch in multiple parts. Once a certain memory threshold or number of operations have been received, the request can be split and submitted for processing.

This commit splits bulks once memory usage for indexing pressure has passed a configurable threshold.

Integrate the incremental bulks into RestBulkAction

Currently the rest.incremental_bulk is read in two different places. This means that it will be employed in two steps introducing unpredictable behavior. This commit ensures that it is only read in a single place.

The header validator is very aggressive about adjusting autoread on the belief it is the only place where autoread is tweaked. However, with stream backpressure, we should only change it when we are starting or finishing header validation.

Currently, unless a rest handler specifies that it handles "unsafe" buffers, we must copy the http buffers in releaseAndCopy. Unfortuantely, the original content was slipping through in the initial stream PR. This less to memory corruption on index and update requests which depend on buffers being copied.

Currently, the entire close pipeline is not hooked up in case of a channel close while a request is being buffered or executed. This commit resolves the issue by adding a connection to a stream closure.

This commit ensures we properly throw exceptions when an empty bulk request is received with the incremental handling enabled.

Spotless broke during a rebase. Fixing in this commit.

mhl-b

LGTM

This commit back ports all of the work introduced in: #113044 * #111438 - 5e1f655 * #111865 - 478baf1 * #112179 - 1b77421 * #112227 - cbcbc34 * #112267 - c00768a * #112154 - a03fb12 * #112479 - 95b42a7 * #112608 - ce2d648 * #112629 - 0d55dc6 * #112767 - 2dbbd7d * #112724 - 58e3a39 * dce8a0b * #112974 - 92daeeb * 529d349 * #113161 - e3424bd

Tim-Brooks added >non-issue :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v8.16.0 labels Sep 17, 2024

Tim-Brooks requested a review from a team as a code owner September 17, 2024 18:06

elasticsearchmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v9.0.0 labels Sep 17, 2024

Tim-Brooks force-pushed the partial-rest-requests-rebase branch 2 times, most recently from ff66351 to a85a02a Compare September 18, 2024 00:18

Tim-Brooks mentioned this pull request Sep 18, 2024

Implement incremental bulk execution #112956

Closed

nielsbauman reviewed Sep 18, 2024

View reviewed changes

henningandersen approved these changes Sep 18, 2024

View reviewed changes

jbaiera approved these changes Sep 18, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/action/bulk/BulkOperation.java Outdated Show resolved Hide resolved

mhl-b and others added 13 commits September 18, 2024 13:38

Add http request content stream support (#111438)

5e1f655

Allow incremental bulk request execution (#111865)

478baf1

Allow a single bulk request to be passed to Elasticsearch in multiple parts. Once a certain memory threshold or number of operations have been received, the request can be split and submitted for processing.

handle 100-continue and oversized streaming request (#112179)

1b77421

release stream chunk queue on bad request (#112227)

cbcbc34

Split bulks based on memory usage (#112267)

c00768a

This commit splits bulks once memory usage for indexing pressure has passed a configurable threshold.

Incremental bulk integration with rest layer (#112154)

a03fb12

Integrate the incremental bulks into RestBulkAction

Ensure incremental bulk setting is set atomically (#112479)

95b42a7

Currently the rest.incremental_bulk is read in two different places. This means that it will be employed in two steps introducing unpredictable behavior. This commit ensures that it is only read in a single place.

fix leaking listener (#112629)

0d55dc6

Ensure partial bulks released if channel closes (#112724)

58e3a39

Currently, the entire close pipeline is not hooked up in case of a channel close while a request is being buffered or executed. This commit resolves the issue by adding a connection to a stream closure.

merge main

dce8a0b

Properly handle empty incremental bulk requests (#112974)

92daeeb

This commit ensures we properly throw exceptions when an empty bulk request is received with the incremental handling enabled.

Tim-Brooks force-pushed the partial-rest-requests-rebase branch from a326505 to 50eb7f9 Compare September 18, 2024 19:55

Fix spotless in netty stream class

529d349

Spotless broke during a rebase. Fixing in this commit.

Tim-Brooks force-pushed the partial-rest-requests-rebase branch from 50eb7f9 to 529d349 Compare September 18, 2024 19:59

mhl-b approved these changes Sep 18, 2024

View reviewed changes

Tim-Brooks merged commit 3e6acdd into main Sep 18, 2024
16 checks passed

Tim-Brooks deleted the partial-rest-requests-rebase branch September 18, 2024 22:28

Tim-Brooks mentioned this pull request Sep 19, 2024

Backport incremental bulk execution #113215

Merged

ankikuma mentioned this pull request Nov 8, 2024

Fix and add a test for failure store with Incremental bulk #115866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement incremental bulk execution #113044

Implement incremental bulk execution #113044

Uh oh!

Tim-Brooks commented Sep 17, 2024 •

edited

Loading

Uh oh!

elasticsearchmachine commented Sep 17, 2024

Uh oh!

nielsbauman left a comment

Uh oh!

henningandersen left a comment

Uh oh!

henningandersen Sep 18, 2024

Uh oh!

mhl-b Sep 18, 2024 •

edited

Loading

Uh oh!

henningandersen Sep 18, 2024

Uh oh!

Tim-Brooks Sep 18, 2024

Uh oh!

henningandersen commented Sep 18, 2024

Uh oh!

jbaiera left a comment

Uh oh!

Uh oh!

mhl-b left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Implement incremental bulk execution #113044

Implement incremental bulk execution #113044

Uh oh!

Conversation

Tim-Brooks commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 17, 2024

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

mhl-b Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

henningandersen commented Sep 18, 2024

Uh oh!

jbaiera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhl-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Tim-Brooks commented Sep 17, 2024 •

edited

Loading

mhl-b Sep 18, 2024 •

edited

Loading