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

[FEATURE] Create writeable implementations for default value #351

Open
dbwiddis opened this issue Jan 20, 2023 · 2 comments
Open

[FEATURE] Create writeable implementations for default value #351

dbwiddis opened this issue Jan 20, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem?

The Function class used for the defaultValue field in Setting is not Writeable.

What solution would you like?

Create writeable implementations for parser that implement both Function<Settings, String> and Writeable interfaces for all existing setting implementations.

Update WriteableSetting to write the defaultValue function as well as the underlying string (getRaw).

@mloufra
Copy link
Contributor

mloufra commented Feb 15, 2023

  • Add multiple inner classes WriteableDefaultValue class implement Function<Setting, String>, Writeable to make default value is writeable to writeTo and read between Opensearch side and SDK side.
  • Update default value in WriteableSetting class, to add a test to check the default value is instanceof Writeable or not which is similar to issue [FEATURE] Create writeable implementations for parser  #349 (the change make in writeableSetting class).
  • Add test for WriteableDefaultValue.

@dbwiddis
Copy link
Member Author

Some draft code by @mloufra has highlighted a few key facts:

  1. The defaultValue and fallbackSetting parameters are associated with each other: when one exists, the other doesn't.
    • This basically creates a recursive "default" which can be another setting until there is no fallback and the default ends the recursion
    • Testing whether fallbackSetting is null determines which way to go
  2. The existing WriteableSetting implementation does a check for fallback being null when evaluating the setting locally (before sending over transport) but the implementation directly calls intSeting(), etc. which causes immediate evaluation of the fallbacks/defaults and goes straight to the default.
    • This is shown in the (incorrect) WriteableSettingTests class when the getDefault() method of a setting with fallback is shown to evaluate to the same default (tested with a Settings.EMPTY parameter). There is no test for get().
  3. The existing WriteableSettingTests only checks the Settings.EMPTY case which misses the important case where the default/fallback lack of writeability is a problem.

The whole purpose of the fallback is to provide backwards compatibility for an older version of the setting, so users do not have to change their config file, but a new setting name will provide the same functionality. So for example in WriteableSettingTests we define two settings:

  • "intSettingBase" with a default value of 6
  • "intSetting" with a fallback value of the "intSettingBase" setting

We need to test 3 cases:

  1. User does not define their own setting. In this case we want "intSetting" to return the fallback ("intSettingBase") default value of 6.
  2. User defines the old setting "intSettingBase" to a new value (say, 12) but does not define the new setting. In this case we want "intSetting" to return the user-defined "intSettingBase" value (12). This will work before being sent over transport, but will fail after being sent over transport, and instead return the "intSettingBase" default value (6).
  3. User defines the new setting "intSetting". In this case we want "intSetting" to return the user-defined "intSetting" value. This will (probably?) work as expected.

Fixing the case 2 failure is the purpose of this issue.

So the best approach for this PR is to:

  1. Update (fix) the WriteableSettingsTests test method evaluation of fallbacks (in all the test methods) to:
    • Switch the getDefault(Settings.EMPTY) to get(Settings.EMPTY) (which should continue to pass checks). This tests case 1.
    • Add new get(Settings) tests for user-defined settings with the old (base) and new values. See SettingTests for the sample syntax to do this, e.g., Settings.builder().put("a.byte.size", 12).build(). Use the setting names as currently defined for the first parameter (one test for "base" and another for the new value) and make sure the second parameter is different than the default.
      • I expect the tests to fail in case 2 after passing through transport
  2. Fix the code so that the failing tests pass.
    • This might be as simple as wrapping new WriteableSetting() around the existing WriteableSetting check when fallback isn't null.
    • If that doesn't fix it, you may need to redesign a writeable default/fallback class that implements the above logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants