Skip to content

Fixed inconsistent Setting.exist() #46603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 16, 2019
Merged

Fixed inconsistent Setting.exist() #46603

merged 6 commits into from
Sep 16, 2019

Conversation

@probakowski
Copy link
Contributor

@probakowski probakowski commented Sep 11, 2019 •
edited
Loading

Setting.exist() will no longer return true if there's setting in keystore with the same name.

Closes #41830

@probakowski
Fixed inconsistent Setting.exist
15b419b 
Setting.exist will no longer return true if there's setting in keystore with the same name.
Also removed synchronization on keySet to lower congestion, possibly constructing
keySet multiple times, which should be cheap though.

Closes elastic#41830
@probakowski probakowski added >enhancement :Core/Infra/Settings Settings infrastructure and APIs v8.0.0 labels Sep 11, 2019
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 11, 2019

Pinging @elastic/es-core-infra

@rjernst
Copy link
Member

rjernst commented Sep 11, 2019

I think having this alternate version of keySet will further confuse things. Please see my comments on the original issue. We should be able to make the behavior consistent within the current methods.

@probakowski
Copy link
Contributor Author

probakowski commented Sep 11, 2019 •
edited
Loading

I've seen the comments there, but I don't see how we can get both get and exist behave consistently with current methods.
Setting.get and SecureSetting.get now do the same thing - throw exception when setting is in wrong place (keystore or yaml respectively) after checks added in #42209
Still, Setting.exist will be happy with both keystore (where get will throw exception) and yaml, where SecureSetting.exist will only accept keystore. As we don't have public access right now to unsecure settings only, we can't fix Setting.exist. So we can either:

  1. "Break" SecureSetting.exist to behave as Setting.exist - can be done without adding/opening anything but doesn't look like good idea
  2. Add Settings.unsecureKeySet that skip secure settings
  3. Add Settings.keySet with parameter as in this PR
  4. Add explicit Settings.containsSecureSetting and Settings.containsUnsecureSetting and skip keySet altogether in both exist methods
    For me the best is either 3 or 4, WDYT?

@rjernst
Copy link
Member

rjernst commented Sep 12, 2019

I'm ok with the keySet method as in this PR, but I don't think it needs to be public? The Settings class for a node is a representation of all settings across elasticsearch.yml and the keystore. This is an implementation detail only necessary to get correct behavior for this one method.

@probakowski
Copy link
Contributor Author

probakowski commented Sep 12, 2019

@rjernst you were right that there is option to reuse current methods, using reversed check from Setting.innerGetRaw, I think current version is less confusing as we don't open anything new

rjernst
rjernst approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit, then LGTM

server/src/main/java/org/elasticsearch/common/settings/Setting.java Outdated
public boolean exists(final Settings settings) {
return settings.keySet().contains(getKey());
SecureSettings secureSettings = settings.getSecureSettings();
return settings.keySet().contains(getKey()) && (secureSettings == null || !secureSettings.getSettingNames().contains(getKey()));
Copy link
Member

@rjernst rjernst Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer to use == false for negations because it is easier to visually spot than !

@probakowski
Copy link
Contributor Author

probakowski commented Sep 13, 2019

@elasticmachine test this please

@probakowski
Copy link
Contributor Author

probakowski commented Sep 13, 2019

@elasticmachine run elasticsearch-ci/2

@probakowski
Copy link
Contributor Author

probakowski commented Sep 13, 2019

@elasticmachine run elasticsearch-ci/bwc

@probakowski
Copy link
Contributor Author

probakowski commented Sep 13, 2019

@elasticmachine run elasticsearch-ci/default-distro

@probakowski
Copy link
Contributor Author

probakowski commented Sep 16, 2019

@elasticmachine run elasticsearch-ci/docs

@probakowski probakowski merged commit faf6ef8 into elastic:master Sep 16, 2019
@probakowski probakowski deleted the setting-exist branch September 16, 2019 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjernst rjernst rjernst approved these changes

@tvernum tvernum Awaiting requested review from tvernum

Assignees

No one assigned

Labels

:Core/Infra/Settings Settings infrastructure and APIs >enhancement v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Setting.exists reflects secure settings but .get doesnt

4 participants

@probakowski @elasticmachine @rjernst @jakelandis