opensearch-sdk-java
opensearch-sdk-java copied to clipboard
[FEATURE] Create writeable implementations for default value
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).
- [ ] Add multiple inner classes WriteableDefaultValue class implement
Function<Setting, String>,Writeableto makedefault valueis writeable towriteToandreadbetween Opensearch side and SDK side. - [ ] Update default value in
WriteableSettingclass, to add a test to check the default value isinstanceofWriteable or not which is similar to issue #349 (the change make inwriteableSettingclass). - [ ] Add test for
WriteableDefaultValue.
Some draft code by @mloufra has highlighted a few key facts:
- The
defaultValueandfallbackSettingparameters 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
fallbackSettingis null determines which way to go
- The existing
WriteableSettingimplementation does a check for fallback being null when evaluating the setting locally (before sending over transport) but the implementation directly callsintSeting(), etc. which causes immediate evaluation of the fallbacks/defaults and goes straight to the default.- This is shown in the (incorrect)
WriteableSettingTestsclass when thegetDefault()method of a setting with fallback is shown to evaluate to the same default (tested with aSettings.EMPTYparameter). There is no test forget().
- This is shown in the (incorrect)
- The existing
WriteableSettingTestsonly checks theSettings.EMPTYcase 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:
- User does not define their own setting. In this case we want "intSetting" to return the fallback ("intSettingBase") default value of 6.
- 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).
- 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:
- Update (fix) the
WriteableSettingsTeststest method evaluation of fallbacks (in all the test methods) to:- Switch the
getDefault(Settings.EMPTY)toget(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. SeeSettingTestsfor 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
- Switch the
- 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
- This might be as simple as wrapping