Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detach SearchPhases from AbstractSearchAsyncAction #23118

Merged
merged 8 commits into from Feb 14, 2017

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

commented Feb 10, 2017

Today all search phases are inner classes of AbstractSearchAsyncAction or one of it's
subclasses. This makes unittesting of these classes practically impossible. This commit
Extracts DfsQueryPhase and FetchSearchPhase or of the code that composes the actual
query execution types and moves most of the fan-out and collect code into an InitialSearchPhase
class that can be used to build initial search phases (phases that retry on shards). This will
make modification to these classes simpler and allows to easily compose or add new search phases
down the road if additional roundtrips are required.

Detach SearchPhases from AbstractSearchAsyncAction
Today all search phases are inner classes of AbstractSearchAsyncAction or one of it's
subclasses. This makes unittesting of these classes practically impossible. This commit
Extracts `DfsQueryPhase` and `FetchSearchPhase` or of the code that composes the actual
query execution types and moves most of the fan-out and collect code into an `InitialSearchPhase`
class that can be used to build initial search phases (phases that retry on shards). This will
make modification to these classes simpler and allows to easily compose or add new search phases
down the road if additional roundtrips are required.

s1monw added some commits Feb 10, 2017

fix visibiltiy of methods in AbstractSearchAsyncAction, add javadocs …
…and use SetOnce on lazy initialization
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

@elasticmachine would you bother to test this

@colings86
Copy link
Member

left a comment

LGTM, but I think a second pair of eyes form @jpountz or @jimczi would be good here too

private final SearchPhaseContext context;
private final SearchTransportService searchTransportService;

DfsQueryPhase(AtomicArray<DfsSearchResult> firstResults,

This comment has been minimized.

Copy link
@colings86

colings86 Feb 13, 2017

Member

Should we call this dfsResults instead of firstResults so its clear what the results contain?

this.request = request;
this.shardsIts = shardsIts;
this.logger = logger;
// we need to add 1 for non active partition, since we count it in the total!

This comment has been minimized.

Copy link
@colings86

colings86 Feb 13, 2017

Member

What do you mean by the non-active partition here?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

@colings86 I pushed new commits to address your comments.

@jimczi

jimczi approved these changes Feb 13, 2017

Copy link
Member

left a comment

This is a great change that introduces unit tests for each phase and simplifies things in a complicated way ;) !
The abstraction for search phase and the chaining are just great and you preserved small optims like query and fetch for single shard query.
Looks fantastic to me.

});
}
}
protected SearchPhase getNextPhase(AtomicArray<DfsSearchResult> results, SearchPhaseContext context) {

This comment has been minimized.

Copy link
@jimczi

jimczi Feb 13, 2017

Member

beautiful !

@jpountz
Copy link
Contributor

left a comment

LGTM. It is hard to do thorough reviews for such changes, but I like where this is going. I left some nitpicks about moving assertions in non performance-sensitive code to hard exceptions.

if (hadOne) {
sb.append(",");
} else {
hadOne = true;
}

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 13, 2017

Contributor

minor suggestion, feel free to ignore, but maybe StringJoiner would make it easier to read

this.counter = new CountDown(expectedOps);
this.onFinish = onFinish;
this.context = context;
assert expectedOps <= resultArray.length() :

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 13, 2017

Contributor

let's make it a hard exception?

this.context = context;
this.logger = context.getLogger();
assert context.getNumShards() == queryResults.length() : "number of shards must match the length of the query results but doesn't:"
+ context.getNumShards() + "!=" + queryResults.length();

This comment has been minimized.

Copy link
@jpountz

jpountz Feb 13, 2017

Contributor

let's make it a hard exception too?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

@jpountz @jimczi thanks for the feedback I pushed fixes for the last suggestions and will merge this once it's build.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

@elasticmachine would you bother to test this

1 similar comment
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

@elasticmachine would you bother to test this

@s1monw s1monw merged commit aef0665 into elastic:master Feb 14, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:break_out_initial_search_phase branch Feb 14, 2017

s1monw added a commit that referenced this pull request Feb 14, 2017

Detach SearchPhases from AbstractSearchAsyncAction (#23118)
Today all search phases are inner classes of AbstractSearchAsyncAction or one of it's
subclasses. This makes unit testing of these classes practically impossible. This commit
Extracts `DfsQueryPhase` and `FetchSearchPhase` or of the code that composes the actual
query execution types and moves most of the fan-out and collect code into an `InitialSearchPhase`
class that can be used to build initial search phases (phases that retry on shards). This will
make modification to these classes simpler and allows to easily compose or add new search phases
down the road if additional roundtrips are required.

s1monw added a commit that referenced this pull request Feb 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.