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

Refactor of query profile classes to make way for other profile implementations #18370

Merged
merged 1 commit into from May 16, 2016

Conversation

Projects
None yet
4 participants
@colings86
Copy link
Member

commented May 16, 2016

To prepare for #10538 this PR refactors some of the profiler classes to allow them to be reused by other profile types. It also renames other classes to make it clear they relate to Query profiling to avoid confusion when another profiler is added later.

@colings86

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

@jpountz @polyfractal would you mind reviewing this?

@colings86 colings86 added the >breaking label May 16, 2016

@jpountz

This comment has been minimized.

Copy link
Contributor

commented May 16, 2016

LGTM

private static final ParseField QUERY_TYPE = new ParseField("query_type");
private static final ParseField LUCENE_DESCRIPTION = new ParseField("lucene");
private static final ParseField TYPE = new ParseField("type");
private static final ParseField DESCRIPTION = new ParseField("description");

This comment has been minimized.

Copy link
@colings86

colings86 May 16, 2016

Author Member

Breaking change is here as the output of the profiler will slightly change

@colings86 colings86 force-pushed the colings86:refactor/query-profiler branch May 16, 2016

@polyfractal

This comment has been minimized.

Copy link
Member

commented May 16, 2016

LGTM!

@polyfractal

This comment has been minimized.

Copy link
Member

commented May 16, 2016

Rename looks good too.

Not crazy about the placement of buildShardResults() into Profilers, but not quite sure where to put it. That class only deals with query profiling bits (adding/removing to the current context, etc), whereas I'm expecting buildShardResults() will accumulate aggregation profiling bits too, since it is helping to construct the final result object.

Maybe it should go in SearchProfileShardResults, since it is a helper for assembling the final results? Dunno. Just a minor thing though, I think it's fine to merge without the change too.

@polyfractal

This comment has been minimized.

Copy link
Member

commented May 16, 2016

For posterity, chatted with @colings86 on slack: Profilers will be growing new methods to support agg profiling, so my comment was mostly moot :)

@colings86 colings86 force-pushed the colings86:refactor/query-profiler branch to e37e8af May 16, 2016

@colings86 colings86 merged commit 83df20b into elastic:master May 16, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@colings86 colings86 deleted the colings86:refactor/query-profiler branch May 16, 2016

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.