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

HLRC: Add rollup search #36334

Merged
merged 2 commits into from
Dec 7, 2018
Merged

HLRC: Add rollup search #36334

merged 2 commits into from
Dec 7, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Dec 6, 2018

Relates to #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@@ -93,6 +94,20 @@ static Request deleteJob(final DeleteRollupJobRequest deleteRollupJobRequest) th
return request;
}

static Request search(final SearchRequest request) throws IOException {
if (request.types().length > 0) {
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I look at this the less I like it. I think the other option is to build a HLRC side request object that uses the server-side agg builders. I'm not sure if that is worth it, but it would be a cleaner API. It wouldn't be exactly the API we want in the end, but it'd be better than this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hub-cap pointed me to HLRC's CountRequest which is pretty similar to what I'm describing. Different because instead of rendering a SearchSourceBuilder it we'd just do aggregations and queries and stuff like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern but I find it easier if the request object is the same since you can directly use it for rollup or regular search seamlessly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that argument too!

I think in an ideal world the request for search and the request for rollup search would implement the same interface or have a common superclass....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hub-cap, what do you think about merging this like it stands now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just placeholder the client side object and test the validation there instead? just extend the existing request? if not then im ok merging as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't extend SearchRequest because it is final. I'll merge as is then. I think it'd be nice to make the client side search request objects for rollup search at the same time as we make them for search. One day, one day....

@@ -26,6 +26,7 @@ and rewrites it back to what a client would expect given the original query.
indices.

Rules for the `index` parameter:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page was rendering badly without this.

import java.util.Set;

public class RestRollupSearchAction extends BaseRestHandler {

private static final Set<String> RESPONSE_PARAMS = Collections.singleton(RestSearchAction.TOTAL_HIT_AS_INT_PARAM);
private static final Set<String> RESPONSE_PARAMS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required to support the HLRC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

NumericMetricsAggregation.SingleValue maxTemperature =
response.getAggregations().get("max_temperature");
assertThat(maxTemperature.value(), closeTo(49.0, .00001));
// end::search-response-response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// end::search-response ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh. I wonder why that didn't make the build fail.... Let me check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I suspect it doesn't fail because the tag checker ignores tailing characters. It probably should only ignore trailing spaces to be honest. But that is a job for another day!

import java.util.Set;

public class RestRollupSearchAction extends BaseRestHandler {

private static final Set<String> RESPONSE_PARAMS = Collections.singleton(RestSearchAction.TOTAL_HIT_AS_INT_PARAM);
private static final Set<String> RESPONSE_PARAMS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@@ -93,6 +94,20 @@ static Request deleteJob(final DeleteRollupJobRequest deleteRollupJobRequest) th
return request;
}

static Request search(final SearchRequest request) throws IOException {
if (request.types().length > 0) {
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern but I find it easier if the request object is the same since you can directly use it for rollup or regular search seamlessly.

@nik9000 nik9000 merged commit ead2b9e into elastic:master Dec 7, 2018
@nik9000
Copy link
Member Author

nik9000 commented Dec 7, 2018

I'm running the full suite of tests on this backport locally so I added the tag. It'll take a few hours.

@nik9000
Copy link
Member Author

nik9000 commented Dec 7, 2018

And I need the full suite because I touched both the rollup code and the client.

nik9000 added a commit that referenced this pull request Dec 8, 2018
jasontedor added a commit to liketic/elasticsearch that referenced this pull request Dec 9, 2018
* elastic/6.x: (37 commits)
  [HLRC] Added support for Follow Stats API (elastic#36253)
  Exposed engine must have all ops below gcp during rollback (elastic#36159)
  TEST: Always enable soft-deletes in ShardChangesTests
  Use delCount of SegmentInfos to calculate numDocs (elastic#36323)
  Add soft-deletes upgrade tests (elastic#36286)
  Remove LocalCheckpointTracker#resetCheckpoint (elastic#34667)
  Option to use endpoints starting with _security (elastic#36379)
  [CCR] Restructured QA modules (elastic#36404)
  RestClient: on retry timeout add root exception (elastic#25576)
  [HLRC] Add support for put privileges API (elastic#35679)
  HLRC: Add rollup search (elastic#36334)
  Explicitly recommend to forceMerge before freezing (elastic#36376)
  Rename internal repository actions to be internal (elastic#36377)
  Core: Remove parseDefaulting from DateFormatter (elastic#36386)
  [ML] Prevent stack overflow while copying ML jobs and datafeeds (elastic#36370)
  Docs: Fix Jackson reference (elastic#36366)
  [ILM] Fix issue where index may not yet be in 'hot' phase (elastic#35716)
  Undeprecate /_watcher endpoints (elastic#36269)
  Docs: Fix typo in bool query (elastic#36350)
  HLRC: Add delete template API (elastic#36320)
  ...
@nik9000
Copy link
Member Author

nik9000 commented Dec 10, 2018

All backported!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants