-
Notifications
You must be signed in to change notification settings - Fork 25.4k
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
Conversation
Pinging @elastic/es-analytics-geo (:Analytics/Rollup) |
d4ad68e
to
fbf53c3
Compare
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.
fbf53c3
to
d287131
Compare
server/src/main/java/org/elasticsearch/cluster/metadata/RollupGroup.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/RollupGroup.java
Show resolved
Hide resolved
} else if ("false".equals(property)) { | ||
ROLLUPV2_FEATURE_FLAG_REGISTERED = false; | ||
} else if (property == null) { | ||
ROLLUPV2_FEATURE_FLAG_REGISTERED = null; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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!
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
server/src/main/java/org/elasticsearch/cluster/metadata/RollupGroup.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/time/WriteableZoneId.java
Outdated
Show resolved
Hide resolved
// 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
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>
This commit lowers the minimum compatiblity of RollupMetadata since elastic#64892 was merged into 7.11. relates to elastic#64680.
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