Skip to content
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

Allow index settings behavior to depend on index version #67621

Open
jtibshirani opened this issue Jan 16, 2021 · 5 comments
Open

Allow index settings behavior to depend on index version #67621

jtibshirani opened this issue Jan 16, 2021 · 5 comments
Labels
:Core/Infra/Settings Settings infrastructure and APIs >enhancement Team:Core/Infra Meta label for core/infra team

Comments

@jtibshirani
Copy link
Contributor

Our index compatibility policy requires that indices created in major version n-1 continue to work in version n without manual intervention, which means we must be able to read index metadata that was valid in n-1. We haven't always followed this policy closely for index settings. It hasn't been a big problem in the past, perhaps because we archive all unknown and invalid settings when a node starts up. We plan to remove this leniency on start-up and would like to uphold clearer compatibility guarantees.

To support this, the behavior of index settings must be able to depend on the index version. That way if a setting value is deprecated in 7.x, we can reject it for new 8.x indices but continue to allow it for 7.x indices (maybe falling back to another value). Perhaps there should be a mechanism for validating settings that has access to the index version. Note this set-up would only be relevant to 'index scoped' settings, not Setting objects in general.

@jtibshirani jtibshirani added >enhancement :Core/Infra/Settings Settings infrastructure and APIs labels Jan 16, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 16, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jan 21, 2021

In our discussion, we noted that the index version is technically already available in Validator#validate, since we pass in the complete settings (which includes the version). We decided to work through examples in order to determine exactly when/ how the version is needed. Here are some recent index settings changes I'm aware of (note that by accident we didn't always handle bwc precisely):

In the first two cases, settings were deprecated in n-1 then removed entirely in n. So here we would keep the setting definition, but check the version during validation. In the last two cases, we start to disallow certain values: in n indices the values will be outright rejected, but for n-1 indices we'll accept them and use a fallback. For example, when we encounter fix for index.shard.check_on_startup we would log a warning and use the behavior for false instead. The latter case currently wouldn't be covered by the Validator#validate.

@rjernst
Copy link
Member

rjernst commented Jan 21, 2021

For example, when we encounter fix for index.shard.check_on_startup we would log a warning and use the behavior for false instead. The latter case currently wouldn't be covered by the Validator#validate.

In this case, I think the calling code would need to do the translation. So the code that looks at index.shard.check_on_startup would see the fix value, and in 8.0 it would change the behavior to act like false. Wdyt @jtibshirani?

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jan 21, 2021

I think it'd be valuable to consolidate + standardize the places where we access the index version (as suggested in #65399), and encapsulating this in the settings framework would be in line with that goal. Some downsides I see with the calling code being responsible:

  • Any time the setting is accessed, the caller needs to remember to apply matching compatibility logic. Settings could be accessed in multiple places.
  • We end up with index version checks scattered in different components. This makes it harder to figure out what exactly the behavior is for different index versions. It also hurts code quality since compatibility concerns can pop up in any place.

@rjernst
Copy link
Member

rjernst commented Jan 25, 2021

While I understand the concern over duplication of the compatibility logic, in practice I think we usually have a single place reading settings. If there are multiple places needing to read the same setting, we can share utility code to read that setting.

However, I'm not sure what I suggested here would require compatibility logic on the read side in this case. With the case of the fix value, the code reading the setting would continue to have logic for the fix value. The limitation of which indexes can contain the fix value would be limited to the validator. Then once there are no longer any indices that could legitimately use the fix value, the reading code can be adjusted to no longer need this case.

I don't think we should complete the settings infrastructure with Version, since not all settings are index settings. We could in theory create wrappers for all index setting types, but I think that would be quite complex for the relatively few number of cases we have for modifying the behavior for compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Settings Settings infrastructure and APIs >enhancement Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

3 participants