Unify logsdb index settings providers #118342
Conversation
kkrik-es
added
>non-issue
Team:StorageEngine
:StorageEngine/Logs
kkrik-es
added
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
Looks good!
But I think there is an issue if the new index settings provider returns index.mode setting and user template uses index.mode setting as well. In that case we would always overrule index.mode=logsdb, while the user may have opted out of logsdb?
| return Settings.builder().put("index.mode", IndexMode.LOGSDB.getName()).build(); | ||
| settingsBuilder = Settings.builder().put(IndexSettings.MODE.getKey(), IndexMode.LOGSDB.getName()); | ||
| if (supportSyntheticSourceDemotion()) { | ||
| indexTemplateAndCreateRequestSettings = Settings.builder() |
There was a problem hiding this comment.
Let's also remove index.mode from settingsBuilder if indexTemplateAndCreateRequestSettings contains index.mode setting? I think that is an simpler approach than compared to injecting index.mode=logsdb into indexTemplateAndCreateRequestSettings
Otherwise we would enforce index.mode=logsdb even if user template define index.mode?
There was a problem hiding this comment.
This is copied from:
I suggest we keep this as pure refactoring, we can improve the logic separately.
Btw, don't we check for index.mode in settings in line 84 above?
| createdIndexVersion.set(indexVersion); | ||
| } | ||
|
|
||
| private boolean supportSyntheticSourceDemotion() { |
There was a problem hiding this comment.
Maybe rename to supportFallbackToStoredSource()? Which is similar to how it has been named before.
|
|
||
| @Override | ||
| public boolean overrulesTemplateAndRequestSettings() { | ||
| // Indicates that the provider value takes precedence over any user setting. |
There was a problem hiding this comment.
This means that both index.mode and index.mapping.source.mode will be overruling if returned by this settings provider.
There was a problem hiding this comment.
Indeed, this is a change. I believe index mode is injected only if it's not present in incoming settings, so there is no delta?
I think the two settings are injected independently, there should be no change in the logic wrt when each gets applied. More so, |
Ah right, I missed that this: |
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Unify logsdb index settings providers * restore diff * rename method (cherry picked from commit c792595) # Conflicts: # x-pack/plugin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsDBPlugin.java
3 participants
Follow-up on #115397, where it was suggested to merge the two providers, to avoid inter-dependencies for provided settings and simplify the logic.