Collapse IndexMode#getConfiguredTimestampRange(...) into IndexMode#getTimestampBound(...)#88258
Conversation
…tTimestampBound(...) `IndexMode#getConfiguredTimestampRange(...)` functionally overlaps a lot with `IndexMode#getTimestampBound(...)`. In order to reuse `IndexMode#getTimestampBound(...)` for the use case `IndexMode#getConfiguredTimestampRange(...)` is reused a change had to be made to TimestampBounds. During query rewrite and in `TimestampFieldMapperService` no `IndexScopedSettings` is available, so made this an optional parameter. Only the usage in `IndexSettings` needs this, so that the instance is updated in the case `index.time_series.end_time` setting is updated. In the case of query rewrite and in `TimestampFieldMapperService` this isn't needed, since the instance is kept around and reused. Followup from elastic#85162
c05577d to
d73200c
Compare
|
Not super happy about the fact that |
| endTime = indexMetadata.getTimeSeriesEnd().toEpochMilli(); | ||
| if (scopedSettings != null) { | ||
| scopedSettings.addSettingsUpdateConsumer(IndexSettings.TIME_SERIES_END_TIME, this::updateEndTime); | ||
| } |
There was a problem hiding this comment.
Ah! I see. What a pain.
I wonder if it's better to have the bounds object be immutable and to move the replacement of the end time to the IndexSettings. Or wrap this in something mutable over there when in tsdb mode. I'd wanted to avoid having a volatile read from IndexSettings for every document write which is why I made this mutable. The way it is now you only do the volatile read when you are in tsdb mode.
The mutability here is annoying. Sorry!
There was a problem hiding this comment.
I tried to the following: ba5460c
Which updates the timestampBounds in IndexSettings if end_time setting changes and makes TimestampBounds immutable.
…imestampRange_into_getTimestampBound
…nds if end_time changes
| if (bounds != null) { | ||
| var indexMode = context.indexSettings().getMode(); | ||
| if (indexMode == IndexMode.TIME_SERIES) { | ||
| TimestampBounds bounds = context.indexSettings().getTimestampBounds(); |
There was a problem hiding this comment.
This avoids doing a volatile read in case of non tsdb indices. Seems like a good idea?
I think this is the only place where getTimestampBounds() getter is being used.
There was a problem hiding this comment.
Nice!
Could you make this context.indexSettings().getMode().validateTimestamp(bounds, first, context)? Or maybe context.indexSettings().getMode().shouldValidateTimestamp()? We're trying to avoid all of the hard checks against indexMode's types. That way you can look at index mode and see "all the things" that it changes.
server/src/main/java/org/elasticsearch/index/IndexSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/IndexSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/IndexSettings.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
It's better than what we had, for sure. A nice compromise. |
IndexMode#getConfiguredTimestampRange(...)functionally overlaps a lot withIndexMode#getTimestampBound(...).In order to reuse
IndexMode#getTimestampBound(...)for the use caseIndexMode#getConfiguredTimestampRange(...)is reused a change had to be made to TimestampBounds.
During query rewrite and in
TimestampFieldMapperServicenoIndexScopedSettingsis available,so made this an optional parameter. Only the usage in
IndexSettingsneeds this, so that theinstance is updated in the case
index.time_series.end_timesetting is updated. In the case ofquery rewrite and in
TimestampFieldMapperServicethis isn't needed, since the instance iskept around and reused.
Followup from #85162