Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TSDB: add index timestamp range check #78291
TSDB: add index timestamp range check #78291
Changes from 4 commits
2bad389
8809757
6c42357
c3f1e66
79e96e0
b4a68c3
63e552c
0e67726
e3b7256
78c132d
fe8f654
120cdaf
563ca87
daccfb7
14a2378
2c45c57
d2a00ec
d631649
1dcebb0
79db882
69df14d
6c77b18
40f2e82
582588f
bfe25c5
6350ee1
a5a116f
7236ee1
20c21a8
9e92d09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think it might be better to do this validation any time the settings are present and just have
IndexMode
make sure the settings are present.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.
when IndexMode is time_series, start_time has a default value:
Instant.ofEpochMilli(0)
, end_time also has a default value:DateUtils.MAX_NANOSECOND_INSTANT
.and these settings can only be set in time_series mode
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.
Yeah. I meant that it's better to run the validation if the max and min are set rather than I the mode is time series. I know one implies the other, but I'd feel more comfortable if the field mapper didn't have to know about time series mode. It only knows about the setting.
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.
You pinged me and asked for another review but haven't change this so I'll restate my objections - I feel like, as much as possible,
IndexMode
should be about making sure that the appropriate settings are set and the appropriate mapping is in place. Then we can let the mapping do the assertions. So I think it's more right to do the test inDataStreamTimestampFieldMapper
and do it something like:I like this because a reader doesn't have to know what time series mode is - they just have to know that they is some way to set the min and max time. If they need to figure out how to set it then they can. But lots of times I find myself not caring where a behavior comes from, just that we have this behavior.