Add copy constructor to SearchRequest #36641
Merged
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
For cross cluster search execution mode (see elastic#32125), we will need to take a search request that spans across multiple clusters (based on index prefixes e.g. cluster1:index, cluster2:index etc.) and split it into multiple search requests to be sent to each cluster. A copy constructor added to SearchRequest would make that easy and well maintanable in the future. Something along the same lines already happens in `BulkByScrollParallelizationHelper`, but the corresponding code went outdated as some new fields were added to `SearchRequest` which were not added to the bulk by scroll code. A copy constructor helps making the task of copying a search request maintainable over time.
javanna
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search |
javanna
commented
Dec 14, 2018
| .indicesOptions(request.indicesOptions()); | ||
| if (request.allowPartialSearchResults() != null) { | ||
| slices[slice].allowPartialSearchResults(request.allowPartialSearchResults()); | ||
| } |
There was a problem hiding this comment.
The following fields were not copied:
this.batchedReduceSize = searchRequest.batchedReduceSize;
this.maxConcurrentShardRequests = searchRequest.maxConcurrentShardRequests;
this.preFilterShardSize = searchRequest.preFilterShardSize;
They look recently added hence I assume this was just a mistake and they should be copied.
nik9000
approved these changes
Dec 14, 2018
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 17, 2018
For cross cluster search alternate execution mode (see elastic#32125), we will need to take a search request that spans across multiple clusters (based on index prefixes e.g. cluster1:index, cluster2:index etc.) and split it into multiple search requests to be sent to each cluster. A copy constructor added to `SearchRequest` would make that easy and well maintainable in the future. Something along the same lines already happens in `BulkByScrollParallelizationHelper`, but the corresponding code went outdated as some new fields were added to `SearchRequest` which were not added to the bulk by scroll code. A copy constructor helps making the task of copying a search request maintainable over time.
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 18, 2018
For cross cluster search alternate execution mode (see elastic#32125), we will need to take a search request that spans across multiple clusters (based on index prefixes e.g. cluster1:index, cluster2:index etc.) and split it into multiple search requests to be sent to each cluster. A copy constructor added to `SearchRequest` would make that easy and well maintainable in the future. Something along the same lines already happens in `BulkByScrollParallelizationHelper`, but the corresponding code went outdated as some new fields were added to `SearchRequest` which were not added to the bulk by scroll code. A copy constructor helps making the task of copying a search request maintainable over time.
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 18, 2018
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 18, 2018
javanna
added a commit
that referenced
this pull request
Dec 18, 2018
For cross cluster search alternate execution mode (see #32125), we will need to take a search request that spans across multiple clusters (based on index prefixes e.g. cluster1:index, cluster2:index etc.) and split it into multiple search requests to be sent to each cluster. A copy constructor added to `SearchRequest` would make that easy and well maintainable in the future. Something along the same lines already happens in `BulkByScrollParallelizationHelper`, but the corresponding code went outdated as some new fields were added to `SearchRequest` which were not added to the bulk by scroll code. A copy constructor helps making the task of copying a search request maintainable over time.
spinscale
pushed a commit
that referenced
this pull request
Dec 19, 2018
javanna
added a commit
that referenced
this pull request
Dec 19, 2018
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.
For cross cluster search alternate execution mode (see #32125) we will need to take a search request that spans across multiple clusters (based on index prefixes e.g. cluster1:index, cluster2:index etc.) and split it into multiple search requests to be sent to each cluster. A copy constructor added to
SearchRequestwould make that easy and well maintainable over time.Something along the same lines already happens in
BulkByScrollParallelizationHelper, but the corresponding code went outdated as some new fields were added toSearchRequestwhich were not added to the bulk by scroll code. A copy constructor should make the need of adjusting it more visible when adding new fields.