Use the remaining scroll response documents on update by query bulk requests #71430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…equests In update by query requests where max_docs < size and conflicts=proceed we weren't using the remaining documents from the scroll response in cases where there were conflicts and in the first bulk request the successful updates < max_docs. This commit address that problem and use the remaining documents from the scroll response instead of requesting a new page. Closes elastic#63671
fcofdez
force-pushed
the
update-by-query-conflicts
branch
from
325c9e8
to
9d8e5b1
Compare
fcofdez
changed the title
fcofdez
changed the title
fcofdez
changed the title
fcofdez
added
Team:Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
...s/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java
Outdated
Show resolved
Hide resolved
...s/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java
Outdated
Show resolved
Hide resolved
modules/reindex/src/test/java/org/elasticsearch/index/reindex/UpdateByQueryConflictTests.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine run elasticsearch-ci/packaging-sample-unix Some of the builds running on centos were timing out yesterday and it seems to be a known problem. |
|
@henningandersen could you take a look into this PR when you have the chance? Thanks! |
| true, | ||
| (bulkByScrollResponse, maxDocs, conflictingUpdates) -> { | ||
| assertThat(bulkByScrollResponse.getUpdated(), is((long) maxDocs)); | ||
| assertThat(bulkByScrollResponse.getVersionConflicts(), is((long) conflictingUpdates)); |
There was a problem hiding this comment.
I think we can move this assertion into the generic test case (executeConcurrentUpdatesOnSubsetOfDocs)
| // Modify a subset of the target documents concurrently | ||
| final List<SearchHit> originalDocs = Arrays.asList(searchResponse.getHits().getHits()); | ||
| final List<SearchHit> docsModifiedConcurrently = new ArrayList<>(); | ||
| if (randomBoolean()) { |
There was a problem hiding this comment.
Can we add a case where we run out of docs on the query, i.e., we end up updating less than max_docs?
| static class ScrollConsumableHitsResponse { | ||
| private final ScrollableHitSource.AsyncResponse asyncResponse; | ||
| private final List<? extends ScrollableHitSource.Hit> hits; | ||
| private final AtomicInteger consumedOffset = new AtomicInteger(0); |
There was a problem hiding this comment.
I think this can just be an int?
There was a problem hiding this comment.
That offset can be updated from different threads, as AbstractAsyncBulkByScrollAction#prepareBulkRequest runs in the GENERIC thread pool, after throttling has been applied.
|
@elasticmachine update branch |
fcofdez
added a commit
to fcofdez/elasticsearch
that referenced
this pull request
Apr 20, 2021
…equests In update by query requests where max_docs < size and conflicts=proceed we weren't using the remaining documents from the scroll response in cases where there were conflicts and in the first bulk request the successful updates < max_docs. This commit address that problem and use the remaining documents from the scroll response instead of requesting a new page. Closes elastic#63671 Backport of elastic#71430
fcofdez
added a commit
that referenced
this pull request
Apr 20, 2021
…equests (#71931) In update by query requests where max_docs < size and conflicts=proceed we weren't using the remaining documents from the scroll response in cases where there were conflicts and in the first bulk request the successful updates < max_docs. This commit address that problem and use the remaining documents from the scroll response instead of requesting a new page. Closes #63671 Backport of #71430
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In update by query requests where max_docs < size and conflicts=proceed
we weren't using the remaining documents from the scroll response in
cases where there were conflicts and in the first bulk request the
successful updates < max_docs. This commit address that problem and
use the remaining documents from the scroll response instead of
requesting a new page.
Closes #63671