-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Fix and unmute testTsdbTemplatesNoKeywordFieldType
#136013
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-storage-engine (Team:StorageEngine) |
Settings.Builder settingsBuilder = Settings.builder() | ||
.put("index.mode", "time_series") | ||
.put("index.dimensions_tsid_strategy_enabled", randomDouble() < 0.8); | ||
if (randomBoolean()) { |
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 this also be always set when the setting above is false?
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'd simply fix as:
boolean dimensions_tsid_strategy_enabled = randomDouble() < 0.8;
...
.put("index.routing_path", dimensions_tsid_strategy_enabled || randomBoolean() ? "metricset" : null)
.put("index.dimensions_tsid_strategy_enabled", dimensions_tsid_strategy_enabled)
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 this also be always set when the setting above is false?
No, even if index.dimensions_tsid_strategy_enabled
is false
, the routing_path
doesn't need to be set explicitly as this is a data stream template, for which the routing_path
will be automatically determined if unset. I explicitly want to test all four permutations of strategy enabled true/false and routing path set/unset. Also, routing path unset is different to routing path explicitly set to null. This triggered the test failure in the first place. It's a bit surprising but not sure if it's worth changing. At least not in this PR.
As both explicitly setting index.routing_path
as well setting index.dimensions_tsid_strategy_enable
to false
disables the new index.dimensions-based tsid strategy, I wanted to give disabling that strategy a lower probability with via randomDouble() < 0.8
.
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.
Note that I also ran this locally with 500 iterations and all have succeeded.
This following was problematic:
Because it set an explicit
null
as the routing path`.In cases where the following evaluates to
false
, this leads to an error rightfully complaining that the routing path is required: