Make AbstractSearchAsyncAction more testable and add a basic test case #20890
Conversation
This file contains hidden or 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
`AbstractSearchAsyncAction` has only been tested in integration tests. The infrastrucutre is rather critical and should be tested on a unittest level. This change takes the first step
s1monw
added
>enhancement
review
:Search/Search
Contributor
Author
nik9000
approved these changes
Oct 13, 2016
Member
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
Left some minor suggestions but LGTM.
| protected abstract String firstPhaseName(); | ||
|
|
||
| protected Executor getExecutor() { | ||
| return threadPool.executor(ThreadPool.Names.SEARCH); |
Member
There was a problem hiding this comment.
Maybe pass the Executor in to the ctor instead of ThreadPool? You could keep this method so you can still override it in tests to throw.
| protected final SearchRequest request; | ||
| protected final ClusterState clusterState; | ||
| protected final DiscoveryNodes nodes; | ||
| protected final Function<String, DiscoveryNode> nodes; |
Member
There was a problem hiding this comment.
Can you add javadoc for this? I'm happy to keep it protected but think it'd be nice to have some note on the protected members about how we expect them to be used. Something as simple as /** Used by subclasses to resolve node ids to DiscoveryNodes. **/ would be nice.
| SearchPhaseController searchPhaseController, ThreadPool threadPool, SearchRequest request, | ||
| ActionListener<SearchResponse> listener) { | ||
| protected AbstractSearchAsyncAction(Logger logger, SearchTransportService searchTransportService, | ||
| Function<String, DiscoveryNode> nodeLookup, Map<String, String[]> perIndexFilteringAliases, |
Member
There was a problem hiding this comment.
Maybe rename nodeLookup to nodeIdToDiscsoveryNode? I love that you've pulled the cluster state stuff out into these functions we can mock out easily in tests but when I first scanned this line I thought "oh, functions that map index name to stuff".
| Map<String, DiscoveryNode> lookup = new HashMap<>(); | ||
| lookup.put(primaryNode.getId(), primaryNode); | ||
| AbstractSearchAsyncAction asyncAction = new AbstractSearchAsyncAction<TestSearchPhaseResult>(logger, transportService, lookup::get, | ||
| Collections.emptyMap(), |
Member
There was a problem hiding this comment.
Can you bump the indentation on the parameters one time so they don't line up with the class body?
Contributor
Author
|
thanks @nik9000 |
s1monw
added a commit
that referenced
this pull request
Oct 13, 2016
#20890) `AbstractSearchAsyncAction` has only been tested in integration tests. The infrastructure is rather critical and should be tested on a unit-test level. This change takes the first step.
clintongormley
added
>test
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 21, 2016
This is a followup to elastic#21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to elastic#9047 (at least for the index closed case). The code block within the change was moved as part of elastic#20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to elastic#21689
javanna
added a commit
that referenced
this pull request
Nov 21, 2016
This is a followup to #21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to #9047 (at least for the index closed case). The code block within the change was moved as part of #20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to #21689
javanna
added a commit
that referenced
this pull request
Nov 21, 2016
This is a followup to #21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to #9047 (at least for the index closed case). The code block within the change was moved as part of #20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to #21689
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
AbstractSearchAsyncActionhas only been tested in integration tests.The infrastrucutre is rather critical and should be tested on a unittest
level. This change takes the first step