[Feature/Extensions] [BUG] SettingsModule#registerDynamicSetting contains unreachable code

[dbwiddis]

Describe the bug

registerDynamicSetting() javadocs state:

Settings can be of Node Scope or Index Scope.

However, this is a "must". And regardless of the scope, some of this code isn't reachable:

OpenSearch/server/src/main/java/org/opensearch/common/settings/SettingsModule.java

Lines 188 to 203 in e08d57b

	public boolean registerDynamicSetting(Setting<?> setting) {
	try {
	registerSetting(setting);
	if (setting.hasNodeScope()) {
	return clusterSettings.registerSetting(setting);
	}
	if (setting.hasIndexScope()) {
	return indexScopedSettings.registerSetting(setting);
	}
	logger.info("Registered new Setting: " + setting.getKey() + " successfully ");
	} catch (Exception e) {
	logger.error("Could not register setting " + setting.getKey());
	throw new SettingsException("Could not register setting:" + setting.getKey());
	}
	return false;
	}

In the case the setting has neither Node nor Index scope, line 190 calls registerSetting() which contains this code:

if (setting.hasNodeScope() || setting.hasIndexScope()) {
    if (setting.hasNodeScope()) {
        // ...
    }
    if (setting.hasIndexScope()) {
        // ...
    }
} else {
    throw new IllegalArgumentException("No scope found for setting [" + setting.getKey() + "]");
}

This jumps immediately to the catch block with an unhelpful log message.

In the case it has Node Scope it passes the first check but returns at line 192.

In the case it has Index Scope and not Node Scope it returns at line 195. However it never reaches this line if it has both Index Scope and Node Scope.

There is no possibility of it ever reaching the log message at line 197.

To Reproduce
Steps to reproduce the behavior:

1. Attempt to register a setting without either Node Scope or Index Scope
2. Observe the failure to register
3. Wonder why, edit the code to produce a stack trace
4. Find the IllegalArgumentExcxeption that gets swallowed in the catch block.

Expected behavior
Log warnings that clearly state why it didn't work (e.g., you didn't have the correct scope). I believe including the caught exception in the re-thrown exception would provide a "Caused By" link to clarify it, but haven't tested this.

[dbwiddis]

The code above is no longer relevant after merge to main. #5495 rewrote the code.

However, tagging @ryanbogan as the new code doesn't seem to match the docs. The javadoc says settings can be either node or index scoped, but they are registered regardless of the scope. Should 'registerSetting(setting)` on line 202 be moved into the conditional?

OpenSearch/server/src/main/java/org/opensearch/common/settings/SettingsModule.java

Lines 185 to 222 in 715ff72

	/**
	* Dynamically registers a new Setting at Runtime. This method is mostly used by plugins/extensions
	* to register new settings at runtime. Settings can be of Node Scope or Index Scope.
	* @param setting which is being registered in the cluster.
	* @return boolean value is set to true when successfully registered, else returns false
	*/
	public boolean registerDynamicSetting(Setting<?> setting) {
	boolean onNodeSetting = false;
	boolean onIndexSetting = false;
	try {
	if (setting.hasNodeScope()) {
	onNodeSetting = clusterSettings.registerSetting(setting);
	}
	if (setting.hasIndexScope()) {
	onIndexSetting = indexScopedSettings.registerSetting(setting);
	}
	try {
	registerSetting(setting);
	if (onNodeSetting || onIndexSetting) {
	logger.info("Registered new Setting: " + setting.getKey() + " successfully ");
	return true;
	}
	} catch (IllegalArgumentException ex) {
	if (onNodeSetting) {
	clusterSettings.unregisterSetting(setting);
	}
	if (onIndexSetting) {
	indexScopedSettings.unregisterSetting(setting);
	}
	throw ex;
	}
	} catch (Exception e) {
	logger.error("Could not register setting " + setting.getKey());
	throw new SettingsException("Could not register setting:" + setting.getKey());
	}
	return false;
	}

And while I'm making requests, can use e.getMessage()for the initial log error at line 218? "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"

[ryanbogan]

@dbwiddis The registerSetting() calls for clusterSettings and indexScopedSettings return booleans representing if the settings are of node or index scope and complete part of the settings registration process. These values are then used for logging if registration is successful, or to unregister the setting (revert the part of the registration done above) if an error occurs at any point in the rest of the settings registration. The remaining registration occurs in the private, static registerSetting() method that is in the try block. Within this method, an IllegalArgumentException is thrown if there is an error or if the setting is not node or index scoped. This exception is then caught by the catch block outside the private method. That being said, I do agree that the SettingsException should contain e.getMessage() instead of the current duplicate phrase, so that should be the action item for this issue.