[BUG] ExtensionsRunner sendAddSettingsUpdateConsumerRequest fails

[dbwiddis]

What is the bug?

ExtensionRunner.sendAddSettingsUpdateConsumerRequest() is failing with unhelpful error messages.

How can one reproduce the bug?

1. In an extension, add a setting using getSettings() extension point:

@Override
public List<Setting<?>> getSettings() {
    // Copied from AnomalyDetectorPlugin getSettings
    List<Setting<?>> enabledSetting = EnabledSetting.getInstance().getSettings();
    List<Setting<?>> systemSetting = ImmutableList
        .of(
            AnomalyDetectorSettings.FILTER_BY_BACKEND_ROLES
        );
    return unmodifiableList(
        Stream
            .of(enabledSetting.stream(), systemSetting.stream())
            .reduce(Stream::concat)
            .orElseGet(Stream::empty)
            .collect(Collectors.toList())
    );
}

2. Start the extension. The setting gets successfully registered:

[2023-01-25T10:20:47,460][INFO ][o.o.c.s.SettingsModule ] [runTask-0] Registered new Setting: plugins.anomaly_detection.filter_by_backend_roles successfully

3. Register the settings update consumer:

sdkClusterService.addSettingsUpdateConsumer(FILTER_BY_BACKEND_ROLES, it -> filterByEnabled = it);

which calls

public <T> void addSettingsUpdateConsumer(Setting<T> setting, Consumer<T> consumer) throws Exception {
    addSettingsUpdateConsumer(Map.of(setting, consumer));
}

which calls

public void addSettingsUpdateConsumer(Map<Setting<?>, Consumer<?>> settingUpdateConsumers) throws Exception {
    extensionsRunner.sendAddSettingsUpdateConsumerRequest(extensionsRunner.getExtensionTransportService(), settingUpdateConsumers);
}

4. Observe log message:

[2023-01-25T10:20:59,429][ERROR][o.o.e.AddSettingsUpdateConsumerRequestHandler] [runTask-0] java.lang.IllegalArgumentException: Setting is not registered for key [plugins.anomaly_detection.filter_by_backend_roles]

What is the expected behavior?

Settings update is registered.

Do you have any additional context?

The root cause of the problem is that the check for existence of the setting is reference equality (note the !=).

public synchronized <T> void addSettingsUpdateConsumer(Setting<T> setting, Consumer<T> consumer, Consumer<T> validator) {
    if (setting != get(setting.getKey())) {
        throw new IllegalArgumentException("Setting is not registered for key [" + setting.getKey() + "]");
    }
    addSettingsUpdater(setting.newUpdater(consumer, logger, validator));
}

But in the AddSettingsUpdateConsumerRequestHandler.handleAddSettingsUpdateConsumerRequest() method, we are extracting the setting argument from the WriteableSetting.

// Extract setting and type from writeable setting
Setting<?> setting = extensionComponentSetting.getSetting();

// Register setting update consumer with callback method to extension
clusterService.getClusterSettings().addSettingsUpdateConsumer(setting, (data) -> {

While the settings are .equals() (if that method existed) to each other, they are != to each other's reference.

Additional bug: as noted in this comment the update request is only looking in the cluster settings (node scoped) but is not also looking in the index scoped settings (index scoped) object, so this also needs to be fixed.

Proposed fix

It might be that the reference equality is a bug and this bug could be fixed by changing it to .equals(). However, I am not aware whether the reference equality is an intentional thing.

As an alternative workaround, or in addition to fixing the equality check, the handleAddSettingsUpdateConsumerRequest() code above could be modified to look up the actual setting from the key passed in by the consumer. Even better, I'm not sure we need to go to the whole effort of sending the entire setting over transport in the AddSettingsUpdateConsumerRequest. Just sending the String key should be sufficient here.

Also copying a comment from this issue, please address: opensearch-project/OpenSearch#4531 (comment)

use e.getMessage() for the initial log error at (OpenSearch SettingsModule) line 218-219? "Could not register setting plugins.anomaly_detection.ad_result_history_rollover_period" is not nearly as helpful as "Cannot register setting [plugins.anomaly_detection.ad_result_history_rollover_period] twice"

[mloufra]

While the settings are .equals() to each other, they are != to each other's reference.

@dbwiddis The operator == compares two objects have the same memory location and equals() compares two objects for content equality. Based on these two objects between != (setting and get(setting.getKey())) is going to check they have the same memory location, so I think we do not really need to change and the problem may be that the memory locations of the two objects are not the same.

[mloufra]

But if this if statement is going to compare content, then I think it should be use equals() instead of !=, it will be more reasonable.

[mloufra]

Based on my understand, the if statement if (setting != get(setting.getKey())) is to use get method get the key from setting and use this to this key value is storage in Map keySetting or not, and check this setting is Group Setting or not, based on this setting is groupSetting or not, it will return two different settings or null if the setting can not be found. one is regular setting, another one is if the setting contains a nested object it will use startwith method to check if the given key starts with the string we are searching for.

[dbwiddis]

But if this if statement is going to compare content, then I think it should be use equals() instead of !=, it will be more reasonable.

If you want to do this, you'd need to implement equals() in the Setting class, and I'm not sure that's an easy task.

One thing to think about, is how all these settings were created in the first place (start with the getSettings() extension point and follow what happens after sendRegisterCustomSettingsRequest() which is received on OpenSearch side by CustomSettingsRequestHandler. Tracing through that code it looks like there are maps which store the setting with the key as the string, and you should be able to retrieve the actual original setting just with the key.

[dbwiddis]

Based on my understand, the if statement if (setting != get(setting.getKey())) is to use get method get the key from setting

As you've seen, this particular get() method is a bit more complicated. It's the same setting but stored in a different location. See previous comment for a better approach that should get the identical (reference) setting.

[Kuanysh-kst]

Hello @dbwiddis, I would like to take this issue

[Kuanysh-kst]

@dbwiddis I have a question, In which class to put this code:

 @Override
public List<Setting<?>> getSettings() {
    // Copied from AnomalyDetectorPlugin getSettings
    List<Setting<?>> enabledSetting = EnabledSetting.getInstance().getSettings();
    List<Setting<?>> systemSetting = ImmutableList
        .of(
            AnomalyDetectorSettings.FILTER_BY_BACKEND_ROLES
        );
    return unmodifiableList(
        Stream
            .of(enabledSetting.stream(), systemSetting.stream())
            .reduce(Stream::concat)
            .orElseGet(Stream::empty)
            .collect(Collectors.toList())
    );
}

In opensearch-sdk-java and in Opensearch there is no EnabledSetting class and it confuses me

[dbwiddis]

Hey @Kuanysh-kst sorry for the slow reply. I'm moving and am online far less than usual.

The getSettings() method is an extension point that would be mapped in the top level extension (in our case, the AnomalyDetectorExtension class, which you'd find on the feature/extensions branch in that repo, where we're doing all our work. EnabledSetting is specific to the AD extension and I actually haven't looked into detail what it does, as it's not really needed for this bug fix.

For this PR you can probably use our Hello World extension as a starting point; we have the getSettings() method already in the HelloWorldExtensions class so you could just declare a class where you add a settings update consumer for that existing setting.

[minalsha]

Hi @Kuanysh-kst please do share if you have any other question. Hopefully above response from @dbwiddis helps you with working on this issue. Thanks

[Kuanysh-kst]

hello, I'm working on this issue

[dbwiddis]

Hey @Kuanysh-kst just summarizing a lot of the above conversation to simplify things and hopefully help you:

• the reproducing case in the initial issue is already existing code in the Anomaly Detection extension on the feature/extensions branch that we are working with so you wouldn't need to add anything if you just ran that extension
  • alternately, as I posted in my latest comment, you could simply add an update consumer for the setting registered in the SDK's "hello world" example
• I mentioned using .equals() but that's not really possible because that has not been added to the Settings class and would be a lot more work. So going the route of using the key for the existing original setting seems to be the best choice.
  • While you could use the key from the WriteableSetting you extract, it's even simpler if you only pass the (String) key when you are registering the update to begin with!
    • But we also need to know the properties, or at least whether it's index or node scoped, so perhaps we should not do this simplification yet.
• The hard/tricky part of this fix is finding which map to use to extract the actual instance of the setting object that got registered.
  • The registration starts at SettingsModule.registerDynamicSetting, but from there, there are different paths for index or node scoped settings, and each of those has two possible maps to register in.
    • This actually uncovers a second bug!!! Our settings update request is only checking for settings in the ClusterSettings (node scoped) but not in the IndexScopedSettings.
    • The equality comparison uses get(setting.getKey()) (on the abstract parent class of both the ClusterSettings and IndexScopedSettings) which also points to a possible way of getting the object. If you have the ClusterSettings instance you should be able to just pass the key to that get() method to get the setting. (If you get null, return the "not registered" error, otherwise you're doing an update.)
      • DO the same for the IndexScopedSettings

[minalsha]

hi @Kuanysh-kst are you still working on this issue? Do you need any help?

[minalsha]

@dbwiddis can we close this issue since the PR: opensearch-project/OpenSearch#7456 is merged?