Refactor WLM settings to use Setting objects and rename field by dzane17 · Pull Request #21143 · opensearch-project/OpenSearch

dzane17 · 2026-04-06T19:52:18Z

Description

This PR refactors the workload group settings introduced in #20536. The search_settings field (added in 3.6 as @ExperimentalApi) is replaced with a settings field backed by the OpenSearch Settings framework instead of a raw Map<String, String>.

Changes:

Rename the JSON field from "search_settings" to "settings"
Rename the timeout key from "timeout" to "search.default_search_timeout" to match the cluster setting name
Replace Map<String, String> with Settings objects internally
Mark WorkloadGroupSearchSettings and WorkloadGroup.getSettings() as @ExperimentalApi

Backward compatibility (3.6 → 3.7 rolling upgrade)

Settings configured on 3.6 workload groups do not carry forward to 3.7. Users must re-apply settings using the new "settings" field and "search.default_search_timeout" key after upgrade. This is permitted because search_settings was marked @ExperimentalApi.
Serialization is not broken during rolling upgrade. A 3.7 node sends and receives empty/dummy values when communicating with 3.6 nodes to prevent deserialization errors. The wire format remains structurally valid in both directions.

Testing

Create group with settings

curl -s -X PUT localhost:9200/_wlm/workload_group -H 'Content-Type: application/json' -d '{
  "name": "test_group",
  "resiliency_mode": "enforced",
  "resource_limits": {"cpu": 0.3, "memory": 0.3},
  "settings": {"search.default_search_timeout": "30s"}
}'

{
    "_id": "Tt2a5BZhR7yvEyLmkBiLOg",
    "name": "test_group",
    "resiliency_mode": "enforced",
    "resource_limits": { "cpu": 0.3, "memory": 0.3 },
    "settings": { "search.default_search_timeout": "30s" },
    "updated_at": 1775177784440
}

Update settings

curl -s -X PUT localhost:9200/_wlm/workload_group/test_group -H 'Content-Type: application/json' -d '{
  "settings": {"search.default_search_timeout": "1m"}
}'

{
    "_id": "Tt2a5BZhR7yvEyLmkBiLOg",
    "name": "test_group",
    "resiliency_mode": "enforced",
    "resource_limits": { "cpu": 0.3, "memory": 0.3 },
    "settings": { "search.default_search_timeout": "1m" },
    "updated_at": 1775177797926
}

Create group without settings

curl -s -X PUT localhost:9200/_wlm/workload_group -H 'Content-Type: application/json' -d '{
  "name": "no_settings_group",
  "resiliency_mode": "soft",
  "resource_limits": {"cpu": 0.2, "memory": 0.2}
}'

{
    "_id": "dXoLfTh3QzmA54o5OHEMNA",
    "name": "no_settings_group",
    "resiliency_mode": "soft",
    "resource_limits": { "cpu": 0.2, "memory": 0.2 },
    "settings": {},
    "updated_at": 1775177804834
}

Old field name "search_settings" rejected

curl -s -X PUT localhost:9200/_wlm/workload_group -H 'Content-Type: application/json' -d '{
  "name": "bad_group",
  "resiliency_mode": "soft",
  "resource_limits": {"cpu": 0.1, "memory": 0.1},
  "search_settings": {"timeout": "30s"}
}'

{
    "error": {
        "root_cause": [{
            "type": "illegal_argument_exception",
            "reason": "search_settings is not a valid object in WorkloadGroup"
        }],
        "type": "illegal_argument_exception",
        "reason": "search_settings is not a valid object in WorkloadGroup"
    },
    "status": 400
}

Old key name "timeout" rejected

curl -s -X PUT localhost:9200/_wlm/workload_group -H 'Content-Type: application/json' -d '{
  "name": "bad_group2",
  "resiliency_mode": "soft",
  "resource_limits": {"cpu": 0.1, "memory": 0.1},
  "settings": {"timeout": "30s"}
}'

{
    "error": {
        "root_cause": [{
            "type": "illegal_argument_exception",
            "reason": "Unknown WLM setting: timeout"
        }],
        "type": "illegal_argument_exception",
        "reason": "Unknown WLM setting: timeout"
    },
    "status": 400
}

Invalid time value rejected

curl -s -X PUT localhost:9200/_wlm/workload_group -H 'Content-Type: application/json' -d '{
  "name": "bad_group3",
  "resiliency_mode": "soft",
  "resource_limits": {"cpu": 0.1, "memory": 0.1},
  "settings": {"search.default_search_timeout": "not_a_time"}
}'

{
    "error": {
        "root_cause": [{
            "type": "illegal_argument_exception",
            "reason": "Invalid value 'not_a_time' for search.default_search_timeout: failed to parse setting [search.default_search_timeout] with value [not_a_time] as a time value: unit is missing or unrecognized"
        }],
        "type": "illegal_argument_exception",
        "reason": "Invalid value 'not_a_time' for search.default_search_timeout: failed to parse setting [search.default_search_timeout] with value [not_a_time] as a time value: unit is missing or unrecognized"
    },
    "status": 400
}

Update resource_limits only — settings preserved

curl -s -X PUT localhost:9200/_wlm/workload_group/test_group -H 'Content-Type: application/json' -d '{
  "resource_limits": {"cpu": 0.4, "memory": 0.4}
}'

{
    "_id": "Tt2a5BZhR7yvEyLmkBiLOg",
    "name": "test_group",
    "resiliency_mode": "enforced",
    "resource_limits": { "cpu": 0.4, "memory": 0.4 },
    "settings": { "search.default_search_timeout": "1m" },
    "updated_at": 1775244892902
}

Clear settings with empty object

curl -s -X PUT localhost:9200/_wlm/workload_group/test_group -H 'Content-Type: application/json' -d '{
  "settings": {}
}'

{
    "_id": "Tt2a5BZhR7yvEyLmkBiLOg",
    "name": "test_group",
    "resiliency_mode": "enforced",
    "resource_limits": { "cpu": 0.4, "memory": 0.4 },
    "settings": {},
    "updated_at": 1775177841951
}

Related Issues

Part of #20555

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-04-06T19:53:43Z

PR Reviewer Guide 🔍

(Review updated until commit `e480464`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes Sub-PR theme: Add optional Settings stream read/write helpers Relevant files: server/src/main/java/org/opensearch/common/settings/Settings.java Sub-PR theme: Refactor WLM settings from Map to Settings objects with renamed fields Relevant files: server/src/main/java/org/opensearch/wlm/WorkloadGroupSearchSettings.java server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java server/src/main/java/org/opensearch/cluster/metadata/WorkloadGroup.java server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java server/src/test/java/org/opensearch/wlm/WorkloadGroupSearchSettingsTests.java server/src/test/java/org/opensearch/cluster/metadata/WorkloadGroupTests.java server/src/test/java/org/opensearch/cluster/metadata/WorkloadGroupMetadataTests.java server/src/test/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListenerTests.java Sub-PR theme: Update plugin-level tests for renamed WLM settings fields Relevant files: plugins/workload-management/src/javaRestTest/java/org/opensearch/rest/WorkloadManagementRestIT.java plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/WorkloadManagementTestUtils.java plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/action/CreateWorkloadGroupResponseTests.java plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/action/GetWorkloadGroupResponseTests.java plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/action/UpdateWorkloadGroupResponseTests.java
⚡ Recommended focus areas for review Wire Format Bug In the legacy 3.6 write path (`writeTo`), when `out.getVersion().onOrAfter(Version.V_3_6_0)` but before V_3_7_0, the code writes `out.writeBoolean(false)` followed by `out.writeMap(Map.of(), ...)`. However, the original 3.6 format wrote `out.writeBoolean(searchSettings == null)` followed by the map only if non-null. Writing `false` (meaning "not null") and then an empty map is a valid encoding, but it differs from what a real 3.6 node would write when settings are null (which would be `true` with no map). This asymmetry could cause deserialization issues when a 3.7 node sends to a 3.6 node that then forwards to another 3.6 node. if (out.getVersion().onOrAfter(Version.V_3_7_0)) { Settings.writeOptionalSettingsToStream(settings, out); } else if (out.getVersion().onOrAfter(Version.V_3_6_0)) { // Legacy 3.6 format: write empty map (experimental API, settings not preserved across versions) out.writeBoolean(false); out.writeMap(Map.of(), StreamOutput::writeString, StreamOutput::writeString); } Default Value Semantics `WLM_SEARCH_TIMEOUT` is defined with a default of `TimeValue.MINUS_ONE`. In `WorkloadGroupRequestOperationListener`, the check `wlmSettings.hasValue(...)` guards against applying the default, but if a user explicitly sets `"search.default_search_timeout": "-1"`, `hasValue` returns true and `MINUS_ONE` would be applied as a timeout, which may have unintended behavior depending on how the search framework interprets a -1 timeout. This edge case should be validated or documented. public static final Setting<TimeValue> WLM_SEARCH_TIMEOUT = Setting.timeSetting("search.default_search_timeout", TimeValue.MINUS_ONE); Null Settings Getter The `getSettings()` method can return `null` if the no-arg constructor `MutableWorkloadGroupFragment()` is used (e.g., during XContent parsing before `setSettings` is called). Callers like `WorkloadGroup.getSettings()` and the XContent serializer handle this with null checks, but it is fragile. Consider initializing `settings` to `Settings.EMPTY` in the no-arg constructor. public MutableWorkloadGroupFragment() {} Incomplete Test `testLegacySearchSettingsFieldRejected` verifies that parsing a JSON with `search_settings` throws an `IllegalArgumentException` containing "search_settings". However, the current `WorkloadGroup.fromXContent` implementation may silently ignore unknown fields rather than throwing, depending on how the parser is configured. The test should be verified to actually fail before this fix and pass after, to confirm the behavior is intentional and not just a parser quirk. public void testLegacySearchSettingsFieldRejected() throws IOException { String json = "{\"_id\":\"test_id\",\"name\":\"test\",\"resiliency_mode\":\"enforced\"," + "\"resource_limits\":{\"memory\":0.5}," + "\"search_settings\":{\"timeout\":\"30s\"}," + "\"updated_at\":1720047207}"; XContentParser parser = createParser(JsonXContent.jsonXContent, json); IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> WorkloadGroup.fromXContent(parser)); assertTrue(exception.getMessage().contains("search_settings")); }

github-actions · 2026-04-06T19:54:17Z

PR Code Suggestions ✨

Latest suggestions up to e480464

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against applying a "no timeout" sentinel value The `WLM_SEARCH_TIMEOUT` setting has a default value of `TimeValue.MINUS_ONE`, so `setting.get(wlmSettings)` will never throw for a missing key — it returns the default. The `hasValue` check correctly guards against applying the default, but if the timeout resolves to `TimeValue.MINUS_ONE` (meaning "no timeout"), it should not be applied to the search request. Add a check to skip applying the timeout when it equals `TimeValue.MINUS_ONE`. server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java [75-85] Settings wlmSettings = workloadGroup.getSettings(); if (wlmSettings != null && wlmSettings.hasValue(WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.getKey())) { try { TimeValue timeout = WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.get(wlmSettings); - if (searchRequest.source() != null && searchRequest.source().timeout() == null) { + if (timeout != null && !timeout.equals(TimeValue.MINUS_ONE) + && searchRequest.source() != null && searchRequest.source().timeout() == null) { searchRequest.source().timeout(timeout); } } catch (Exception e) { logger.error("Failed to apply workload group settings", e); } } Suggestion importance[1-10]: 7 __ Why: The `WLM_SEARCH_TIMEOUT` setting defaults to `TimeValue.MINUS_ONE` (no timeout). If a user explicitly sets the value to `-1`, `hasValue` would return true but the timeout should not be applied. This is a valid edge case that could cause unintended behavior.	Medium
General	Remove redundant exception rethrow block The newly added `catch (IllegalArgumentException e) { throw e; }` block is a no-op — rethrowing without modification is redundant and adds noise. It can be removed since uncaught `IllegalArgumentException` from `SearchSettingsParser.parseField` will propagate naturally past the `IOException` catch. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [181-184] -} catch (IllegalArgumentException e) { - throw e; } catch (IOException e) { throw new IllegalArgumentException(String.format(Locale.ROOT, "parsing error encountered for the field '%s'", field)); } Suggestion importance[1-10]: 4 __ Why: The `catch (IllegalArgumentException e) { throw e; }` block is indeed redundant since `IllegalArgumentException` is a `RuntimeException` and would propagate past the `IOException` catch naturally. Removing it improves code clarity without changing behavior.	Low
General	Clarify misleading legacy deserialization comment In the legacy 3.6 serialization format, `isNull == true` means `searchSettings` was null (not specified), while `isNull == false` means a map was written and must be read. The current code correctly reads the map when `isNull == false`. However, note that in the old `writeTo`, `out.writeBoolean(searchSettings == null)` writes `true` for null — so `isNull == true` means null was stored and nothing more was written. The logic is correct, but the comment is misleading: `isNull == true` means the map was null (nothing follows), `isNull == false` means a map follows. The comment should be clarified to avoid future bugs. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [72-79] } else if (in.getVersion().onOrAfter(Version.V_3_6_0)) { - // Legacy 3.6 format: read and discard (experimental API, no backward compat guarantee) + // Legacy 3.6 format: boolean true = searchSettings was null (nothing follows), + // boolean false = a string map follows (read and discard). + // Experimental API: no backward compat guarantee, always map to Settings.EMPTY. boolean isNull = in.readBoolean(); if (isNull == false) { in.readMap(StreamInput::readString, StreamInput::readString); } settings = Settings.EMPTY; } Suggestion importance[1-10]: 2 __ Why: This is purely a comment clarification with no functional change. The existing code logic is correct; only the comment wording is improved, which has minimal impact.	Low

Previous suggestions

Suggestions up to commit 348b661

Category	Suggestion	Impact
Possible issue	Guard against applying a "no timeout" sentinel value The `WLM_SEARCH_TIMEOUT` setting has a default value of `TimeValue.MINUS_ONE`, so `setting.get(wlmSettings)` will always return a value (the default) even when the key is not explicitly set. The check `wlmSettings.hasValue(...)` correctly guards against this, but if the timeout resolves to `MINUS_ONE` (meaning "no timeout"), applying it would incorrectly override the absence of a timeout. You should add a guard to skip applying the timeout when it equals `TimeValue.MINUS_ONE`. server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java [75-85] Settings wlmSettings = workloadGroup.getSettings(); if (wlmSettings != null && wlmSettings.hasValue(WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.getKey())) { try { TimeValue timeout = WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.get(wlmSettings); - if (searchRequest.source() != null && searchRequest.source().timeout() == null) { + if (timeout != null && !timeout.equals(TimeValue.MINUS_ONE) + && searchRequest.source() != null && searchRequest.source().timeout() == null) { searchRequest.source().timeout(timeout); } } catch (Exception e) { logger.error("Failed to apply workload group settings", e); } } Suggestion importance[1-10]: 7 __ Why: The `WLM_SEARCH_TIMEOUT` setting defaults to `TimeValue.MINUS_ONE` (no timeout), and if a user explicitly sets it to `-1`, the current code would apply that value. Adding a guard for `MINUS_ONE` prevents incorrectly overriding the absence of a timeout with a "no timeout" sentinel value.	Medium
Possible issue	Fix potential scientific notation in double formatting Using `%s` format specifiers for `double` values (`cpu` and `memory`) relies on the default `toString()` representation, which may produce locale-dependent or scientific notation output (e.g., `4.0E-1`) for certain values. This could cause JSON parsing failures in the REST API. Use `%f` or explicitly format the doubles to avoid unexpected representations. plugins/workload-management/src/javaRestTest/java/org/opensearch/rest/WorkloadManagementRestIT.java [189-198] return String.format(Locale.ROOT, """ { "name": "%s", "resiliency_mode": "%s", "resource_limits": { - "cpu" : %s, - "memory" : %s + "cpu" : %.10f, + "memory" : %.10f }, "settings": {} }""", name, resiliencyMode, cpu, memory); Suggestion importance[1-10]: 5 __ Why: Using `%s` for `double` values can produce scientific notation (e.g., `4.0E-1`) for certain values, which would cause JSON parsing failures. However, the old code also used string concatenation for doubles, and the test values (0.4, 0.2) are unlikely to trigger this issue. Using `%.10f` would add trailing zeros, which may also be undesirable; a better format like `%s` with explicit `Double.toString()` or `%g` might be more appropriate.	Low
General	Clarify legacy deserialization boolean semantics The legacy 3.6 write path wrote `out.writeBoolean(searchSettings == null)` — so `true` means null (no map follows) and `false` means non-null (map follows). The read path correctly reads the map only when `isNull == false`. However, the new `writeTo` for the 3.6 legacy path always writes `false` followed by an empty map, which means nodes on 3.6 reading from a 3.7+ node will always try to read a map. This is consistent, but nodes on 3.7+ reading from a 3.6 node could receive `true` (null settings) and correctly skip the map. The logic appears correct. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [72-79] } else if (in.getVersion().onOrAfter(Version.V_3_6_0)) { - // Legacy 3.6 format: read and discard (experimental API, no backward compat guarantee) + // Legacy 3.6 format: true = null (no map), false = non-null (map follows) boolean isNull = in.readBoolean(); - if (isNull == false) { + if (!isNull) { in.readMap(StreamInput::readString, StreamInput::readString); } settings = Settings.EMPTY; } Suggestion importance[1-10]: 2 __ Why: The suggestion only changes a comment and `isNull == false` to `!isNull`, which is a minor style improvement. The logic is already correct as noted in the suggestion itself, so this has minimal impact.	Low

Suggestions up to commit 2d5a5b1

Category	Suggestion	Impact
Possible issue	Skip applying MINUS_ONE (no-timeout) value to search requests The `WLM_SEARCH_TIMEOUT` setting has a default value of `TimeValue.MINUS_ONE`, so `setting.get(wlmSettings)` will never throw and will return `MINUS_ONE` when the key is absent. However, when the timeout is `MINUS_ONE` (meaning "no timeout"), it should not be applied to the search request. Add a check to skip applying the timeout when its value equals `TimeValue.MINUS_ONE`. server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java [75-85] Settings wlmSettings = workloadGroup.getSettings(); if (wlmSettings != null && wlmSettings.hasValue(WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.getKey())) { try { TimeValue timeout = WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.get(wlmSettings); - if (searchRequest.source() != null && searchRequest.source().timeout() == null) { + if (timeout != null && !timeout.equals(TimeValue.MINUS_ONE) + && searchRequest.source() != null && searchRequest.source().timeout() == null) { searchRequest.source().timeout(timeout); } } catch (Exception e) { logger.error("Failed to apply workload group settings", e); } } Suggestion importance[1-10]: 7 __ Why: This is a valid functional concern: `WLM_SEARCH_TIMEOUT` defaults to `TimeValue.MINUS_ONE`, and applying a "no timeout" value to a search request could have unintended effects. The fix correctly guards against applying `MINUS_ONE` as a timeout value.	Medium
General	Remove redundant exception re-throw block The `catch (IllegalArgumentException e) { throw e; }` block is a no-op re-throw that adds no value and obscures the intent. It was likely added to prevent `IllegalArgumentException` from being caught and wrapped by the `IOException` handler, but since `IllegalArgumentException` is not a subclass of `IOException`, the `IOException` catch would never catch it anyway. This block should be removed to simplify the code. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [188-192] -} catch (IllegalArgumentException e) { - throw e; } catch (IOException e) { throw new IllegalArgumentException(String.format(Locale.ROOT, "parsing error encountered for the field '%s'", field)); } Suggestion importance[1-10]: 4 __ Why: The suggestion is technically correct that `IllegalArgumentException` is not a subclass of `IOException`, so the re-throw block is redundant. However, the block was likely added intentionally to make the intent explicit when `SearchSettingsParser.parseField` throws `IllegalArgumentException` (e.g., from `WorkloadGroupSearchSettings.validate`), so removing it is a minor cleanup with low risk.	Low
General	Clarify legacy deserialization boolean convention In the legacy 3.6 write path, `out.writeBoolean(false)` is written followed by an empty map, meaning the reader will always read `isNull=false` and then read the map. This is consistent. However, if a 3.6 node wrote `isNull=true` (null settings), the 3.7 reader would skip reading the map — which is correct. The logic is sound, but adding a comment clarifying the boolean convention (true=null, false=map follows) would prevent future bugs. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [74-80] } else if (in.getVersion().onOrAfter(Version.V_3_6_0)) { - // Legacy 3.6 format: read and discard (experimental API, no backward compat guarantee) + // Legacy 3.6 format: boolean true means searchSettings was null, false means map follows boolean isNull = in.readBoolean(); - if (isNull == false) { - in.readMap(StreamInput::readString, StreamInput::readString); + if (!isNull) { + in.readMap(StreamInput::readString, StreamInput::readString); // discard legacy map } settings = Settings.EMPTY; } Suggestion importance[1-10]: 2 __ Why: This suggestion only adds a comment clarification and minor style change (`isNull == false` to `!isNull`), which is a very minor readability improvement with no functional impact.	Low

Suggestions up to commit a67bbac

Category	Suggestion	Impact
Possible issue	Guard against applying MINUS_ONE sentinel timeout value The `WLM_SEARCH_TIMEOUT` setting has a default value of `TimeValue.MINUS_ONE`, so `setting.get(wlmSettings)` will always return a value (the default) even when the key is not explicitly set. Using `hasValue` is the correct guard, but you should also skip applying the timeout when the retrieved value equals `TimeValue.MINUS_ONE` (the "no timeout" sentinel), otherwise the default value would be applied as an actual timeout. server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java [75-85] Settings wlmSettings = workloadGroup.getSettings(); if (wlmSettings != null && wlmSettings.hasValue(WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.getKey())) { try { TimeValue timeout = WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.get(wlmSettings); - if (searchRequest.source() != null && searchRequest.source().timeout() == null) { + if (timeout != null && !timeout.equals(TimeValue.MINUS_ONE) + && searchRequest.source() != null && searchRequest.source().timeout() == null) { searchRequest.source().timeout(timeout); } } catch (Exception e) { logger.error("Failed to apply workload group settings", e); } } Suggestion importance[1-10]: 7 __ Why: The `WLM_SEARCH_TIMEOUT` setting defaults to `TimeValue.MINUS_ONE` (no timeout), so if a user explicitly sets the value to `-1`, it would still pass the `hasValue` check and potentially be applied as a timeout. Adding a guard for `TimeValue.MINUS_ONE` is a valid correctness concern, though the impact depends on whether users would explicitly set `-1` as a WLM setting value.	Medium
Possible issue	Preserve null semantics when serializing to older nodes When writing to a 3.6 node, the code always writes `false` (not null) followed by an empty map, discarding any actual settings. However, the 3.6 read path reads `isNull` and then conditionally reads the map. Writing `false` with an empty map is consistent, but the comment says "settings not preserved" which is intentional. The real issue is that if `settings` is `null` (meaning "not specified / keep existing"), writing `false` + empty map will cause the receiving 3.6 node to interpret it as "explicitly set to empty", corrupting the null-vs-empty semantics on the receiving end. Consider writing `true` (null marker) when `settings == null`. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [215-219] } else if (out.getVersion().onOrAfter(Version.V_3_6_0)) { - // Legacy 3.6 format: write empty map (experimental API, settings not preserved across versions) - out.writeBoolean(false); - out.writeMap(Map.of(), StreamOutput::writeString, StreamOutput::writeString); + // Legacy 3.6 format: preserve null marker; settings values not preserved across versions + out.writeBoolean(settings == null); + if (settings != null) { + out.writeMap(Map.of(), StreamOutput::writeString, StreamOutput::writeString); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that always writing `false` (not-null) when serializing to 3.6 nodes loses the null-vs-empty distinction. However, the comment in the code explicitly states "experimental API, settings not preserved across versions," suggesting this is an intentional trade-off. The improved code better preserves null semantics at the cost of slightly more complex logic.	Low

Suggestions up to commit 76b3314

Category	Suggestion	Impact
Possible issue	Guard against applying MINUS_ONE timeout value The `WLM_SEARCH_TIMEOUT` setting has a default value of `TimeValue.MINUS_ONE`, so `Setting.get()` will return `MINUS_ONE` even when the key is not present. However, `hasValue()` checks for an explicitly set value, which is correct. The issue is that when the timeout is `MINUS_ONE` (meaning "no timeout"), it should not be applied to the search request, as applying `-1` as a timeout may have unintended behavior. Add a guard to skip applying the timeout when it equals `TimeValue.MINUS_ONE`. server/src/main/java/org/opensearch/wlm/listeners/WorkloadGroupRequestOperationListener.java [75-85] Settings wlmSettings = workloadGroup.getSettings(); if (wlmSettings != null && wlmSettings.hasValue(WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.getKey())) { try { TimeValue timeout = WorkloadGroupSearchSettings.WLM_SEARCH_TIMEOUT.get(wlmSettings); - if (searchRequest.source() != null && searchRequest.source().timeout() == null) { + if (timeout != null && !timeout.equals(TimeValue.MINUS_ONE) + && searchRequest.source() != null && searchRequest.source().timeout() == null) { searchRequest.source().timeout(timeout); } } catch (Exception e) { logger.error("Failed to apply workload group settings", e); } } Suggestion importance[1-10]: 7 __ Why: This is a valid concern: `TimeValue.MINUS_ONE` means "no timeout" and applying it to a search request could have unintended behavior. The fix correctly adds a guard to skip applying the timeout when it equals `MINUS_ONE`, which is an important correctness issue for the feature's intended behavior.	Medium
Possible issue	Preserve null semantics in legacy serialization format When writing to a 3.6 node, the code always writes `false` (isNull=false) followed by an empty map, discarding the actual settings. However, on the read side for 3.6, `isNull=false` means it will read a map. This is consistent, but the comment says "settings not preserved" which is intentional. The concern is that if `settings` is `null` (meaning "not specified / keep existing"), writing `false` + empty map to a 3.6 node will be interpreted as "explicitly set to empty" upon round-trip, losing the null semantic. Consider writing `true` (isNull=true) when `settings == null` to preserve the null semantic for 3.6 nodes. server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java [215-219] } else if (out.getVersion().onOrAfter(Version.V_3_6_0)) { - // Legacy 3.6 format: write empty map (experimental API, settings not preserved across versions) - out.writeBoolean(false); - out.writeMap(Map.of(), StreamOutput::writeString, StreamOutput::writeString); + // Legacy 3.6 format: preserve null marker, write empty map for non-null settings + out.writeBoolean(settings == null); + if (settings != null) { + out.writeMap(Map.of(), StreamOutput::writeString, StreamOutput::writeString); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that always writing `false` (isNull=false) for 3.6 nodes loses the null semantic of `settings`, which distinguishes "not specified" from "explicitly empty". The improved code preserves this distinction, which matters for the update logic in `updateExistingWorkloadGroup`.	Low
General	Handle null setting values during validation The `value` retrieved via `settings.get(key)` could be `null` if the key exists in the settings but has no value. Passing a `null` value to `Settings.builder().put(key,` `value)` may cause a `NullPointerException` rather than a meaningful `IllegalArgumentException`. Add a null check for `value` before attempting validation. server/src/main/java/org/opensearch/wlm/WorkloadGroupSearchSettings.java [62-67] +if (value == null) { + throw new IllegalArgumentException("Value cannot be null for WLM setting: " + key); +} try { Settings testSettings = Settings.builder().put(key, value).build(); setting.get(testSettings); +} catch (IllegalArgumentException e) { + throw e; } catch (Exception e) { throw new IllegalArgumentException("Invalid value '" + value + "' for " + key + ": " + e.getMessage()); } Suggestion importance[1-10]: 4 __ Why: While the null check for `value` is a reasonable defensive measure, `Settings.get(key)` returning null for an existing key is an edge case that's unlikely in practice with the `Settings` API. The improvement is minor and the existing `catch (Exception e)` would likely handle a NPE anyway.	Low

github-actions · 2026-04-06T20:17:30Z

Persistent review updated to latest commit a67bbac

dzane17 · 2026-04-06T20:45:49Z

@jainankitk @cwperks Can anyone review this PR?

jainankitk

Thanks @dzane17 for this change! Couple of comments on better handling the backward compatibility. Maybe @cwperks can also comment. Also, can we add test for ensuring that adding settings using create/update workload group in the 3.6 format throws an error in 3.7 version?

github-actions · 2026-04-06T21:49:12Z

✅ Gradle check result for a67bbac: SUCCESS

codecov · 2026-04-06T21:50:07Z

Codecov Report

❌ Patch coverage is 71.42857% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.47%. Comparing base (2b482e7) to head (e480464).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/opensearch/wlm/MutableWorkloadGroupFragment.java	60.52%	11 Missing and 4 partials ⚠️
...org/opensearch/cluster/metadata/WorkloadGroup.java	50.00%	1 Missing and 2 partials ⚠️
...steners/WorkloadGroupRequestOperationListener.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21143      +/-   ##
============================================
+ Coverage     73.44%   73.47%   +0.03%     
- Complexity    74456    74464       +8     
============================================
  Files          5967     5967              
  Lines        338232   338230       -2     
  Branches      48755    48749       -6     
============================================
+ Hits         248399   248515     +116     
+ Misses        70075    69870     -205     
- Partials      19758    19845      +87

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dzane17

I had manually tested the old setting names (in PR description). Also added a couple unit tests now.

github-actions · 2026-04-06T22:49:32Z

Persistent review updated to latest commit 2d5a5b1

github-actions · 2026-04-07T00:19:05Z

✅ Gradle check result for 2d5a5b1: SUCCESS

cwperks · 2026-04-18T05:54:02Z

@dzane17 apologies for delay on feedback. I will be taking a look at this in earnest asap (most likely Monday morning).

cwperks · 2026-04-18T05:57:32Z

Rename the JSON field from "search_settings" to "settings"

@dzane17 I'm aligned with this renaming, but is this considered a breaking change? Should this support either legacy key (search_settings) and new key (settings) with a warning for callers using the legacy param that it would be marked for removal?

Edit: Disregard, I see this is addressed in the PR Description

Settings configured on 3.6 workload groups do not carry forward to 3.7. Users must re-apply settings using the new "settings" field and "search.default_search_timeout" key after upgrade. This is permitted because search_settings was marked @experimentalapi.

cwperks

@dzane17 the change looks good to me. 2 things to address:

Use convention where it exists
Remove the CHANGELOG entry since its no longer used

github-actions · 2026-05-04T22:26:16Z

Persistent review updated to latest commit 348b661

Signed-off-by: David Zane <davizane@amazon.com>

github-actions · 2026-05-04T22:33:37Z

Persistent review updated to latest commit e480464

github-actions · 2026-05-04T23:33:09Z

❌ Gradle check result for e480464: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-05T18:56:19Z

❕ Gradle check result for e480464: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

reta · 2026-05-05T20:31:02Z

@cwperks this pull request was merged with the failing checks :( now all pull request fail the breaking changes verification ...

cwperks · 2026-05-05T21:51:20Z

@cwperks this pull request was merged with the failing checks :( now all pull request fail the breaking changes verification ...

@reta my bad I will take a look tn.

Rename the JSON field from "search_settings" to "settings"

This was intentional on this PR and I thought it was the cause of the detect breaking changes failure on this PR.

cwperks · 2026-05-05T21:58:49Z

@dzane17 Can you please help take a look as well?

dzane17 · 2026-05-05T22:14:15Z

@reta @cwperks I opened a PR to add back the public method. Detect Breaking Changes check is passing there: #21500

…arch-project#21143) Signed-off-by: David Zane <davizane@amazon.com>

dzane17 force-pushed the refactor-settings branch from 76b3314 to a67bbac Compare April 6, 2026 20:15

dzane17 marked this pull request as ready for review April 6, 2026 20:16

dzane17 requested a review from a team as a code owner April 6, 2026 20:16

jainankitk reviewed Apr 6, 2026

View reviewed changes

Comment thread server/src/main/java/org/opensearch/cluster/metadata/WorkloadGroup.java

Comment thread server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java

dzane17 force-pushed the refactor-settings branch from a67bbac to 2d5a5b1 Compare April 6, 2026 22:47

dzane17 commented Apr 6, 2026

View reviewed changes

Comment thread server/src/main/java/org/opensearch/wlm/MutableWorkloadGroupFragment.java

Comment thread server/src/main/java/org/opensearch/cluster/metadata/WorkloadGroup.java