Incremental bulk integration with rest layer#112154
Incremental bulk integration with rest layer#112154Tim-Brooks merged 27 commits intoelastic:partial-rest-requestsfrom
Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequestBodyStream.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java
Outdated
Show resolved
Hide resolved
.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java
Outdated
Show resolved
Hide resolved
…emental_bulk_rest
…emental_bulk_rest
| assertResponseException(responseException, "request body is required"); | ||
| } | ||
|
|
||
| @AwaitsFix(bugUrl = "need to decide how to handle this scenario") |
There was a problem hiding this comment.
I have this change in my draft of HttpObjectAggregator removal. It should address no-content partially, for known length I use content-length header, for chunked there is no way to tell so it's always true.
henningandersen
left a comment
There was a problem hiding this comment.
No blocking comments from me, but also did not do a full review. Feel free to merge based on other's reviews.
| final HttpObjectAggregator aggregator = new Netty4HttpAggregator( | ||
| handlingSettings.maxContentLength(), | ||
| httpPreRequest -> enabled.get() == false | ||
| || (httpPreRequest.uri().contains("_bulk") == false || httpPreRequest.uri().contains("_bulk_update")) |
There was a problem hiding this comment.
Can we make this:
| || (httpPreRequest.uri().contains("_bulk") == false || httpPreRequest.uri().contains("_bulk_update")) | |
| || (httpPreRequest.uri().endsWith("/_bulk") == false && httpPreRequest.uri().contains("/_bulk?") == false) |
instead? Seems slightly safer than the anti test for _bulk_update
There was a problem hiding this comment.
I think the uri in this scenario contains query params. But I will look into this. This particular piece of work is quite unfortunate and hopefully we can find a better way either resolving the handler (doing full path analysis) earlier or if we change how we do all aggregation.
There was a problem hiding this comment.
There is even another route that I am having to handle now (old: /_xpack/monitoring/_bulk).
There was a problem hiding this comment.
| final HttpObjectAggregator aggregator = new Netty4HttpAggregator(handlingSettings.maxContentLength()); | ||
| final HttpObjectAggregator aggregator = new Netty4HttpAggregator( | ||
| handlingSettings.maxContentLength(), | ||
| httpPreRequest -> enabled.get() == false |
There was a problem hiding this comment.
I am not entirely sure of the importance of this being in sync with the enabled flag checked in the rest-bulk-handler. However, it seems they could flip at different times, since we register these independently and they each add a listener for cluster settings. I wonder if we can fix that. I realize it is only an issue when turning it on/off, but would be nice to fix anyway.
There was a problem hiding this comment.
I added a comment to the feature flag ticket and will address it as a follow-up. I agree with the concern.
Integrate the incremental bulks into RestBulkAction
Integrate the incremental bulks into RestBulkAction
Integrate the incremental bulks into RestBulkAction
Integrate the incremental bulks into RestBulkAction
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
Integrate the incremental bulks into
RestBulkAction