opensearch-sdk-java icon indicating copy to clipboard operation
opensearch-sdk-java copied to clipboard

[FEATURE] Create writeable implementations for default value

Open dbwiddis opened this issue 2 years ago • 2 comments

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).

dbwiddis avatar Jan 20 '23 20:01 dbwiddis

  • [ ] 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 #349 (the change make in writeableSetting class).
  • [ ] Add test for WriteableDefaultValue.

mloufra avatar Feb 15 '23 18:02 mloufra

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

dbwiddis avatar Feb 27 '23 21:02 dbwiddis