Skip to content

Add RollupV2 cluster metadata behind feature-flag #64680

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

Merged
merged 5 commits into from
Nov 10, 2020

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Nov 6, 2020

this commit sets the foundation for following rollup-v2-related
commits to master.

The intention of this metadata is to be used by both the upcoming
RollupV2 action that will create new indices along with their associated
cluster metadata. This metadata is to be used by the SearchService when
filtering shards in the can-match phase to decide which of the indices
belonging to an original index should be queried.

7.x (7.11) backport: #64892

@talevy talevy added >non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v8.0.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Nov 6, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Rollup)

@talevy talevy mentioned this pull request Nov 6, 2020
this commit sets the foundation for following rollup-v2-related
commits to master.

The intention of this metadata is to be used by both the upcoming
RollupV2 action that will create new indices along with their associated
cluster metadata. This metadata is to be used by the SearchService when
filtering shards in the can-match phase to decide which of the indices
belonging to an original index should be queried.
} else if ("false".equals(property)) {
ROLLUPV2_FEATURE_FLAG_REGISTERED = false;
} else if (property == null) {
ROLLUPV2_FEATURE_FLAG_REGISTERED = null;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just default to false for now? Is there a benefit for making this nullable throughout the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unclear. I kind of chose the "keep feature-flag behavior consistent" route. Instead of asking which is better, I decided to just follow the same pattern that was used by Autoscaling

@talevy talevy requested a review from not-napoleon November 10, 2020 00:50
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM!
I am still not 100% sure if we need more metadata in the cluster state, but we can start with those and change later if needed.
Thanks Tal!

@talevy
Copy link
Contributor Author

talevy commented Nov 10, 2020

@csoulios there will be more that I can think of that I left out for now since my POC was not leveraging them. One detail that is missing: Type of interval - Calendar vs. Fixed. These can be added as needed, will be very minor

A lot of the details needed to decide between different indices in the rollup group tend to be related to which fields are accessible, and for which date interval

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

LGTM

// a map from index-name to the date interval used in the associated index
private Map<String, DateHistogramInterval> dateInterval;
// a map from index-name to timezone used in the associated index
private Map<String, WriteableZoneId> dateTimezone;
Copy link
Member

Choose a reason for hiding this comment

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

Can we condense this down to maybe just one map with a value that holds the interval and the time zone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very much so. Once all the data is being used by the search-resolution code, I suspect the contents of these maps will change 👍 and be simplified into one Map containing a larger Object holding the necessary information in one place

…neId.java

Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>
@talevy talevy merged commit 0a1a28d into elastic:master Nov 10, 2020
@talevy talevy deleted the rollupv2metadata branch November 10, 2020 21:10
@talevy talevy added the v7.11.0 label Nov 10, 2020
talevy added a commit that referenced this pull request Nov 10, 2020
this commit sets the foundation for following rollup-v2-related
commits to master.

The intention of this metadata is to be used by both the upcoming
RollupV2 action that will create new indices along with their associated
cluster metadata. This metadata is to be used by the SearchService when
filtering shards in the can-match phase to decide which of the indices
belonging to an original index should be queried.

Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>
talevy added a commit to talevy/elasticsearch that referenced this pull request Nov 10, 2020
This commit lowers the minimum compatiblity of RollupMetadata
since elastic#64892 was merged into 7.11.

relates to elastic#64680.
@talevy talevy mentioned this pull request Nov 10, 2020
21 tasks
talevy added a commit that referenced this pull request Nov 11, 2020
This commit lowers the minimum compatiblity of RollupMetadata
since #64892 was merged into 7.11.

relates to #64680.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants