Fix dynamic native-memory settings that are accepted but not applied (rebalancer interval/thresholds, spill limit)#22303
Conversation
…ry on dynamic update datafusion.spill_memory_limit_bytes is a Dynamic cluster setting whose SpillLimitValidator depends on datafusion.spill_directory. But spill_directory is NodeScope+Final, so it lives only in opensearch.yml and never reaches cluster state. During a PUT _cluster/settings the framework validates each scope in isolation (SettingsUpdater#updateSettings) against a cluster-state-only view, so the dependency resolves empty even on a node where spill is configured and enabled. The validator then took the "spill disabled" branch and rejected every non-zero limit with "...is non-zero but datafusion.spill_directory is unset", making the cap effectively unsettable via the API on a spill-enabled node. Capture the node-scoped spill directory at startup (createComponents) into a static field and have SpillLimitValidator fall back to it when the dependency map is empty. The volume-capacity check now runs against the real configured directory on a dynamic update: values within the volume are accepted, values exceeding it are rejected with the existing capacity message. Genuine spill-disabled nodes (no directory anywhere) still accept any value as an inert no-op, preserving the startup behavior from opensearch-project#22275. Adds tests for both the dynamic-update accept and reject paths, plus a tearDown that resets the static field between tests. Signed-off-by: snghsvn <snghsvn@amazon.com>
…eed configured thresholds
Two defects in the native-allocator rebalancer settings:
1. native.allocator.rebalance.interval_seconds (dynamic) permanently STOPPED the
rebalancer instead of changing its cadence. updateRebalanceInterval called
cancelRebalanceTask() — which nulls `rebalancer` and `rebalancerScheduler` —
then guarded the reschedule on those same fields being non-null, so the
scheduleAtFixedRate branch was unreachable dead code. A PUT of the interval
was accepted (200) but silently disabled rebalancing until enable was toggled
or the node restarted. Fix: rebuild via startRebalancer using the allocator and
budget supplier captured in buildAllocator; interval 0 still means "disabled".
2. native.allocator.rebalancer.{pressure,idle,shrink}_threshold were ignored at
startup. startRebalancer constructed NativeMemoryRebalancer with
getDefault(Settings.EMPTY) for all three thresholds, so values set in
opensearch.yml were ignored at boot and a disable/enable cycle reset any
applied dynamic values to defaults. Fix: seed from the live ClusterSettings
(yml + applied overrides). The dynamic update consumers were already correct.
Adds package-private test accessors (current rebalancer, task-scheduled flag,
interval-update driver, threshold getters) and unit tests covering both fixes.
Signed-off-by: snghsvn <snghsvn@amazon.com>
…allocator
parquet.native.pool.{write,merge}.min are dynamic settings, but their update
consumers were only registered in the allocator-present branch of
ParquetDataFormatPlugin. In the no-allocator branch only the two MAX consumers
were wired, so a PUT of write/merge pool min was accepted (200) yet silently
dropped.
Pool-min is a virtual-pool floor enforced by the arrow-base allocator's
rebalancer; the raw Rust pools (RustBridge) have only a max, so there is
genuinely nothing to apply a min to without the allocator. Rather than wire a
non-existent Rust min setter, register the MIN consumers in the no-allocator
branch to log a warning that the update has no effect on this node — making the
no-op observable instead of silent, mirroring the spill-limit restart warning.
Signed-off-by: snghsvn <snghsvn@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit ae040cb)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to ae040cb Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit cce1014
Suggestions up to commit e613fdb
Suggestions up to commit e613fdb
Suggestions up to commit b025b72
Suggestions up to commit 5c2e80d
|
|
❌ Gradle check result for 749f33c: 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? |
|
Persistent review updated to latest commit 749f33c |
…terSettings.get() The rebalancer rebuild path (startRebalancer, and the enable consumer) re-read sibling settings via clusterSettings.get(PRESSURE/IDLE/SHRINK_THRESHOLD) and get(REBALANCE_INTERVAL). During a PUT _cluster/settings apply cycle, get() resolves against lastSettingsApplied, which the framework only swaps in AFTER all update consumers have run — so a single PUT that changes the interval (or toggles enabled) together with a threshold rebuilt the rebalancer with the STALE pre-PUT threshold. It only self-corrected because the threshold consumers happen to be registered after the interval consumer and ran later in the same batch — correctness by incidental ordering, not by design. Replace the cross-setting get() calls with volatile fields: pressureThreshold, idleThreshold, shrinkFactor, rebalanceIntervalSeconds. Seeded from node settings at startup (buildAllocator — not mid-cycle, so reading `settings` is correct) and kept current by the same update consumers. startRebalancer and the enable consumer now seed from these fields, so a combined PUT rebuilds with the correct new values regardless of consumer ordering. Adds testCombinedIntervalAndThresholdPutSeedsRebuiltRebalancerWithNewThreshold, which drives a real applySettings() cycle changing interval+threshold together and asserts the rebuilt rebalancer carries the new threshold. Signed-off-by: snghsvn <snghsvn@amazon.com>
|
Persistent review updated to latest commit 151172f |
|
❌ Gradle check result for 151172f: 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? |
|
Persistent review updated to latest commit 4ecff02 |
…e aggregate partition group_by_spills_to_disk_and_returns_correct_results failed intermittently in CI with "Failed to reserve memory for sort during spill: ResourcesExhausted ... for GroupedHashAggregateStream[1]". Root cause: the test ran target_partitions=2, so two GroupedHashAggregateStreams shared one 12 MiB DynamicLimitPool. Below the 16 MiB RSS gate that pool is pure greedy/first-come-first-serve (== GreedyMemoryPool). When a partition spills it frees its own state and then reserves ~32 KiB of sort headroom (datafusion row_hash.rs `spill()`); if a sibling partition has already greedily filled the pool, that 32 KiB reservation fails and the spill errors. Whether partition 0 fills the pool before partition 1 grabs its headroom depends on thread scheduling, so the failure was nondeterministic. DataFusion's own docs say to use FairSpillPool (not greedy) when multiple operators all spill, and its aggregate spill tests do exactly that. Set target_partitions=1: a single aggregator frees its own state before reserving sort headroom, so there is no cross-partition starvation and spill is deterministic. The ~500k-group hash table still dwarfs the 12 MiB pool, so the test still forces a real spill through the production pool to disk and verifies result correctness — exactly what it exists to prove. Signed-off-by: snghsvn <snghsvn@amazon.com>
|
Persistent review updated to latest commit dfd1e12 |
|
Persistent review updated to latest commit 69b9d10 |
… native allocator" This reverts commit 749f33c. Signed-off-by: snghsvn <snghsvn@amazon.com>
|
Persistent review updated to latest commit deefeb4 |
|
Persistent review updated to latest commit deefeb4 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22303 +/- ##
============================================
- Coverage 73.45% 73.35% -0.10%
+ Complexity 76195 76157 -38
============================================
Files 6076 6076
Lines 345767 345785 +18
Branches 49761 49762 +1
============================================
- Hits 253968 253647 -321
- Misses 71531 71913 +382
+ Partials 20268 20225 -43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Persistent review updated to latest commit 5c2e80d |
|
Persistent review updated to latest commit b025b72 |
|
Persistent review updated to latest commit e613fdb |
|
Persistent review updated to latest commit e613fdb |
gaurav-amz
changed the title
|
Persistent review updated to latest commit cce1014 |
|
Persistent review updated to latest commit ae040cb |
1 participant
Fix 1 — Changing the rebalancer interval silently turned the rebalancer off
Setting:
native.allocator.rebalance.interval_seconds(arrow-base)updateRebalanceInterval()first calledcancelRebalanceTask(), which sets therebalancerandrebalancerSchedulerfields tonull. It then tried to reschedule — but guarded that reschedule on those same fields being non-null. They were just nulled, so the reschedule branch was unreachable dead code.The visible effect: you could
PUTa new interval and get a200, but the rebalancer never ran again. It stayed off until someone toggled theenabledflag or restarted the node.Fix: rebuild the rebalancer through
startRebalancer(), using the allocator and budget supplier that were captured at startup. An interval of0still means "disabled", as before.Fix 2 — The rebalancer ignored thresholds set in
opensearch.ymlSettings:
native.allocator.rebalance.{pressure_threshold, idle_threshold, shrink_factor}(arrow-base)startRebalancer()built theNativeMemoryRebalancerusinggetDefault(Settings.EMPTY)for these three thresholds.Settings.EMPTYhas nothing in it, so this always returned the hard-coded defaults — any value you put inopensearch.ymlwas thrown away at boot, and a disable/enable cycle reset live values back to defaults.Fix: seed the thresholds from the live
ClusterSettings(which includes bothopensearch.ymlvalues and any applied dynamic overrides) instead of from an empty settings object.Fix 3 — A valid spill limit was rejected on a node where spill is actually enabled
Setting:
datafusion.spill_memory_limit_bytes(analytics-backend-datafusion)This setting's validator checks the requested limit against the spill volume's real capacity. To find the volume, it depends on
datafusion.spill_directory. The catch:spill_directoryisNodeScope + Final, so it lives only inopensearch.ymland never appears in cluster state.When you do a dynamic
PUT _cluster/settings, OpenSearch validates each setting scope in isolation, so the validator's dependency onspill_directorycame back empty — even on a node where spill is fully configured. The validator then assumed "spill is disabled" and rejected any non-zero limit with a misleading "spill_directory is unset" error.Fix: capture the node-scoped spill directory once at startup, and have the validator fall back to it when the dependency map is empty. Now the capacity check runs against the real configured volume, so valid limits are accepted and only genuinely-too-large values are rejected (with the correct "exceeds spill volume capacity" message).
Related Issues
N/A — internal audit findings.
Check List
--signoff.