Skip to content

Unified native allocator framework to track and monitor Arrow allocations - #21703

Merged
Bukhtawar merged 2 commits into
opensearch-project:mainfrom
Bukhtawar:unified-arrow-allocator
May 19, 2026
Merged

Unified native allocator framework to track and monitor Arrow allocations#21703
Bukhtawar merged 2 commits into
opensearch-project:mainfrom
Bukhtawar:unified-arrow-allocator

Conversation

@Bukhtawar

@Bukhtawar Bukhtawar commented May 17, 2026

Copy link
Copy Markdown
Contributor

Introduces a new plugin (native-allocator-arrow) that owns a single Arrow RootAllocator for the node, with named pool-level children for each subsystem (flight, query, datafusion, ingest). A background rebalancer redistributes unused capacity across pools every 5 seconds so active pools can burst beyond their guarantee when others are idle.

The SPI (libs/arrow-spi) defines the Arrow-agnostic interface, pool config constants, and Writeable stats shape. The plugin implements it with Arrow's BufferAllocator and registers configurable limits as dynamic cluster settings.

This is a framework-only PR — no existing code is modified. Follow-up PRs will wire consumers (arrow-flight-rpc, analytics-engine, analytics-backend-datafusion, parquet-data-format) to use the unified allocator and expose stats via _nodes/stats.

libs/arrow-spi/  (depends on libs:core only — NO Arrow)
  ├── NativeMemoryAllocator.java      ← interface: getOrCreatePool, setPoolLimit, stats()
  ├── NativeMemoryPoolConfig.java     ← pool names, setting keys, static defaults
  └── NativeMemoryPoolStats.java      ← Writeable + ToXContent stats blob

  plugins/native-allocator-arrow/  (depends on libs:arrow-spi + arrow-memory-core)
  ├── ArrowNativeMemoryAllocator.java ← implements NativeMemoryAllocator using RootAllocator
  └── NativeAllocatorArrowPlugin.java ← registers settings from yaml, creates pools on startup
                                         exposes allocator as a component

  plugins/arrow-flight-rpc/        ← extendedPlugins = ['native-allocator-arrow']
  plugins/analytics-engine/        ← extendedPlugins = ['native-allocator-arrow']
  plugins/analytics-backend-datafusion/ ← extendedPlugins = ['native-allocator-arrow']
  plugins/parquet-data-format/     ← extendedPlugins = ['native-allocator-arrow']

The indexing path now flows through, other flows need to wired similarly

  NativeAllocatorArrowPlugin (root)
    └── "ingest" pool (limit from yaml)
          └── ArrowBufferPool.createChildAllocator("vsr-N")
                └── ManagedVSR (per-batch Arrow buffers)

Needs to be integrated with #21465

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

@Bukhtawar
Bukhtawar requested a review from a team as a code owner May 17, 2026 12:24
@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 953673d.

PathLineSeverityDescription
plugins/native-allocator-arrow/build.gradle17highNew external dependency added: org.apache.arrow:arrow-memory-core at version interpolated from versions.arrow. Per mandatory rule, all new external dependency additions must be flagged for maintainer verification regardless of apparent legitimacy.
plugins/native-allocator-arrow/build.gradle18highNew external dependency added: org.apache.arrow:arrow-memory-netty at version interpolated from versions.arrow. Per mandatory rule, all new external dependency additions must be flagged for maintainer verification regardless of apparent legitimacy.
plugins/native-allocator-arrow/build.gradle19highNew external dependency added: org.apache.arrow:arrow-memory-netty-buffer-patch at version interpolated from versions.arrow. Per mandatory rule, all new external dependency additions must be flagged for maintainer verification regardless of apparent legitimacy.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 3 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from a2e1c78 to 953673d Compare May 17, 2026 12:37
@Bukhtawar Bukhtawar added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label May 17, 2026
@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 953673d to f2877b6 Compare May 17, 2026 13:20
@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 670cb36)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The rebalancer thread pool is created with a single thread but never properly shut down if an exception occurs during construction. If new RootAllocator(rootLimit) throws, the executor remains running. This leaks a daemon thread and its resources.

public ArrowNativeAllocator(long rootLimit) {
    this.root = new RootAllocator(rootLimit);
    org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor executor =
        new org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor(1, r -> {
            Thread t = new Thread(r, "native-allocator-rebalancer");
            t.setDaemon(true);
            return t;
        });
    executor.setRemoveOnCancelPolicy(true);
    this.rebalancer = executor;
    INSTANCE = this;
}
Race Condition

The singleton INSTANCE is set before the allocator is fully initialized. If another thread calls instance() between line 83 and the completion of the constructor (e.g., during pool creation in ArrowBasePlugin.createComponents), it may observe a partially constructed allocator with uninitialized fields like rebalanceTask or incomplete pool setup.

    INSTANCE = this;
}
Possible Issue

In rebalance(), the headroom calculation rootLimit - totalAllocated can underflow if totalAllocated exceeds rootLimit due to concurrent allocations or limit changes. The Math.max(0, ...) prevents negative headroom, but the subsequent division headroom / activeCount will yield zero, causing active pools to be capped at their min even when they need more memory. This can starve active workloads when the root is temporarily over-allocated.

long headroom = Math.max(0, rootLimit - totalAllocated);
int activeCount = activePoolNames.size();
long bonusPerActive = activeCount > 0 ? headroom / activeCount : 0;
Possible Issue

validateMinSum checks for overflow by comparing sum < prev, but this only detects overflow if the sum wraps to a negative value. If two large positive long values sum to a smaller positive value (e.g., Long.MAX_VALUE + 1 wraps to Long.MIN_VALUE, which is negative, but Long.MAX_VALUE/2 + Long.MAX_VALUE/2 + 2 wraps to a small positive), the check may miss the overflow. A safer approach is to check sum - min < prev or use Math.addExact.

long sum = 0;
for (long min : mins) {
    long prev = sum;
    sum += min;
    if (sum < prev) {
        throw new IllegalArgumentException("Sum of pool minimums overflows.");
    }
}
Resource Leak

If ArrowNativeAllocator.instance() throws IllegalStateException, a standalone RootAllocator is created as a fallback. However, if an exception occurs later in the constructor (e.g., during limit / 10 if limit is somehow invalid), the RootAllocator is never closed, leaking native memory. The constructor should use try-catch to ensure cleanup.

    BufferAllocator alloc;
    boolean owns = false;
    try {
        alloc = ArrowNativeAllocator.instance().getPoolAllocator(NativeAllocatorPoolConfig.POOL_INGEST);
    } catch (IllegalStateException e) {
        alloc = new RootAllocator(Long.MAX_VALUE);
        owns = true;
        logger.warn("NativeAllocator not available, using standalone RootAllocator");
    }
    this.poolAllocator = alloc;
    this.ownsAllocator = owns;
    long limit = poolAllocator.getLimit();
    this.maxChildAllocation = limit == Long.MAX_VALUE ? Long.MAX_VALUE : limit / 10;
    logger.debug("ArrowBufferPool: limit={}, maxChildAllocation={}", limit, maxChildAllocation);
}

@github-actions

github-actions Bot commented May 17, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 670cb36

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Cancel rebalance task before shutdown

The close() method doesn't cancel the scheduled rebalance task before shutting down
the executor. If a rebalance is running when shutdownNow() is called, it may
continue accessing pools that are being closed, causing race conditions. Cancel
rebalanceTask first if it exists.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
+    ScheduledFuture<?> task = rebalanceTask;
+    if (task != null) {
+        org.opensearch.common.util.concurrent.FutureUtils.cancel(task);
+    }
     rebalancer.shutdownNow();
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 8

__

Why: Valid race condition fix. The close() method should cancel the scheduled rebalanceTask before shutting down the executor to prevent the rebalance operation from accessing pools that are being closed concurrently.

Medium
Fix overflow detection in sum validation

The overflow check if (sum < prev) only detects overflow when the sum wraps to a
negative value. However, if individual min values are Long.MAX_VALUE, the addition
can overflow without becoming negative. Use Math.addExact() to properly detect all
overflow cases.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-205]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
-        long prev = sum;
-        sum += min;
-        if (sum < prev) {
+        try {
+            sum = Math.addExact(sum, min);
+        } catch (ArithmeticException e) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
     }
     if (sum > rootLimit) {
         throw new IllegalArgumentException(
             "Sum of pool minimums ("
                 + sum
                 + " bytes) exceeds root limit ("
                 + rootLimit
                 + " bytes). "
                 + "Reduce pool minimums or increase "
                 + NativeAllocatorPoolConfig.SETTING_ROOT_LIMIT
         );
     }
 }
Suggestion importance[1-10]: 7

__

Why: The current overflow check if (sum < prev) may not detect all overflow cases, particularly when adding Long.MAX_VALUE. Using Math.addExact() provides more robust overflow detection, though the practical impact is limited since pool minimums are unlikely to be Long.MAX_VALUE.

Medium

Previous suggestions

Suggestions up to commit 481de66
CategorySuggestion                                                                                                                                    Impact
Possible issue
Cancel rebalance task before shutdown

The close() method doesn't cancel the scheduled rebalance task before shutting down
the executor. If a rebalance is running when shutdownNow() is called, it may
continue accessing pools that are being closed, causing race conditions. Cancel
rebalanceTask before shutting down the executor.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
+    if (rebalanceTask != null) {
+        org.opensearch.common.util.concurrent.FutureUtils.cancel(rebalanceTask);
+        rebalanceTask = null;
+    }
     rebalancer.shutdownNow();
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 8

__

Why: Valid race condition concern. Canceling the rebalanceTask before shutting down the executor prevents the rebalance task from accessing pools during closure, improving thread safety during shutdown.

Medium
Handle Long.MAX_VALUE in overflow check

The overflow check if (sum < prev) doesn't account for the case where individual min
values are Long.MAX_VALUE. When adding Long.MAX_VALUE to any positive number, the
result wraps to a negative value, which would incorrectly pass the overflow check.
Validate that individual min values are not Long.MAX_VALUE before summing.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-205]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
+        if (min == Long.MAX_VALUE) {
+            throw new IllegalArgumentException("Pool minimum cannot be Long.MAX_VALUE");
+        }
         long prev = sum;
         sum += min;
         if (sum < prev) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
     }
     if (sum > rootLimit) {
         throw new IllegalArgumentException(
             "Sum of pool minimums ("
                 + sum
                 + " bytes) exceeds root limit ("
                 + rootLimit
                 + " bytes). "
                 + "Reduce pool minimums or increase "
                 + NativeAllocatorPoolConfig.SETTING_ROOT_LIMIT
         );
     }
 }
Suggestion importance[1-10]: 7

__

Why: The overflow detection logic has a subtle flaw when individual min values are Long.MAX_VALUE. Adding explicit validation for Long.MAX_VALUE before summing prevents incorrect overflow detection and improves robustness.

Medium
Suggestions up to commit 8d592a4
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent concurrent modification during rebalance

The rebalance() method iterates over pools without synchronization while concurrent
threads may modify the map via getOrCreatePool() or close(). This can cause
ConcurrentModificationException or inconsistent state. Wrap the iteration and
calculations in a synchronized block or use a snapshot of the pools map.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [199-218]

 void rebalance() {
     if (pools.isEmpty()) return;
 
     long rootLimit = root.getLimit();
     long totalAllocated = 0;
     List<String> activePoolNames = new ArrayList<>();
 
-    for (Map.Entry<String, ArrowPoolHandle> entry : pools.entrySet()) {
+    Map<String, ArrowPoolHandle> poolsSnapshot = new HashMap<>(pools);
+    for (Map.Entry<String, ArrowPoolHandle> entry : poolsSnapshot.entrySet()) {
         long allocated = entry.getValue().allocator.getAllocatedMemory();
         totalAllocated += allocated;
 
         if (allocated > 0) {
             activePoolNames.add(entry.getKey());
         }
     }
 
     long headroom = Math.max(0, rootLimit - totalAllocated);
     int activeCount = activePoolNames.size();
     long bonusPerActive = activeCount > 0 ? headroom / activeCount : 0;
Suggestion importance[1-10]: 8

__

Why: Valid concern about concurrent modification of the pools map during rebalance. The pools field is a ConcurrentHashMap, but iteration can still be affected by concurrent modifications. Creating a snapshot is a reasonable solution to ensure consistency during rebalance calculations.

Medium
Prevent partially constructed singleton exposure

The singleton INSTANCE is set before the allocator is fully initialized. If another
thread accesses instance() during construction, it could observe a partially
constructed object. Move the INSTANCE = this; assignment to the end of the
constructor after all fields are initialized.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [73-84]

+public ArrowNativeAllocator(long rootLimit) {
+    this.root = new RootAllocator(rootLimit);
+    org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor executor =
+        new org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor(1, r -> {
+            Thread t = new Thread(r, "native-allocator-rebalancer");
+            t.setDaemon(true);
+            return t;
+        });
+    executor.setRemoveOnCancelPolicy(true);
+    this.rebalancer = executor;
+    INSTANCE = this;
+}
 
-
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential race condition where INSTANCE is set before full initialization. However, the improved_code is identical to the existing_code, showing no actual change. The issue is valid but the fix isn't demonstrated.

Medium
Fix overflow detection in sum validation

The overflow check sum < prev only detects overflow when the result wraps to a
negative value. If min values are very large but positive, the sum can overflow to a
positive value smaller than prev, which this check misses. Use Math.addExact() or
check Long.MAX_VALUE - prev < min before addition.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-194]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
-        long prev = sum;
-        sum += min;
-        if (sum < prev) {
+        if (Long.MAX_VALUE - sum < min) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
+        sum += min;
     }
Suggestion importance[1-10]: 6

__

Why: The suggestion identifies a subtle overflow detection issue. The current check sum < prev can miss certain overflow scenarios. The improved check Long.MAX_VALUE - sum < min is more robust, though the practical impact is limited since pool minimums are unlikely to cause such edge cases.

Low
General
Wait for rebalancer termination before cleanup

The close() method does not wait for the rebalancer thread to terminate after
shutdownNow(). If the rebalancer is mid-execution when close() is called, it may
access pools or the root allocator after they are closed, causing exceptions. Call
rebalancer.awaitTermination() with a timeout before closing resources.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
     rebalancer.shutdownNow();
+    try {
+        if (!rebalancer.awaitTermination(5, TimeUnit.SECONDS)) {
+            // log warning if timeout
+        }
+    } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+    }
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 7

__

Why: Valid concern about potential race conditions during shutdown. The rebalancer thread could access resources after they're closed. Adding awaitTermination() ensures cleaner shutdown, though the daemon thread and best-effort error handling provide some protection against catastrophic failures.

Medium
Suggestions up to commit 240fc89
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent concurrent modification during rebalance

The rebalance() method iterates over pools without synchronization while concurrent
threads may modify the map via getOrCreatePool() or close(). This can cause
ConcurrentModificationException or inconsistent state. Wrap the iteration in a
synchronized block or use a snapshot of the pools map.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [199-218]

 void rebalance() {
     if (pools.isEmpty()) return;
 
     long rootLimit = root.getLimit();
     long totalAllocated = 0;
     List<String> activePoolNames = new ArrayList<>();
 
-    for (Map.Entry<String, ArrowPoolHandle> entry : pools.entrySet()) {
+    Map<String, ArrowPoolHandle> snapshot = new HashMap<>(pools);
+    for (Map.Entry<String, ArrowPoolHandle> entry : snapshot.entrySet()) {
         long allocated = entry.getValue().allocator.getAllocatedMemory();
         totalAllocated += allocated;
 
         if (allocated > 0) {
             activePoolNames.add(entry.getKey());
         }
     }
 
     long headroom = Math.max(0, rootLimit - totalAllocated);
     int activeCount = activePoolNames.size();
     long bonusPerActive = activeCount > 0 ? headroom / activeCount : 0;
Suggestion importance[1-10]: 8

__

Why: Valid concern about concurrent modification of the pools map during iteration in rebalance(). The ConcurrentHashMap prevents corruption but not ConcurrentModificationException during iteration. Creating a snapshot is a reasonable solution to ensure consistency.

Medium
Prevent partially constructed singleton exposure

The singleton INSTANCE is set before the allocator is fully initialized. If another
thread accesses instance() during construction, it could observe a partially
constructed object. Move the INSTANCE = this; assignment to the end of the
constructor after all fields are initialized.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [73-84]

+public ArrowNativeAllocator(long rootLimit) {
+    this.root = new RootAllocator(rootLimit);
+    org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor executor =
+        new org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor(1, r -> {
+            Thread t = new Thread(r, "native-allocator-rebalancer");
+            t.setDaemon(true);
+            return t;
+        });
+    executor.setRemoveOnCancelPolicy(true);
+    this.rebalancer = executor;
+    INSTANCE = this;
+}
 
-
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential race condition where INSTANCE is set before full initialization. However, the improved_code is identical to existing_code, showing no actual change. The issue is valid but the fix isn't demonstrated.

Medium
General
Wait for executor termination before cleanup

The close() method calls shutdownNow() on the rebalancer but doesn't wait for
termination. If a rebalance task is running, it may continue accessing pools or root
after they are closed, causing exceptions or resource leaks. Call awaitTermination()
after shutdownNow() to ensure the executor has fully stopped.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
     rebalancer.shutdownNow();
+    try {
+        if (!rebalancer.awaitTermination(5, TimeUnit.SECONDS)) {
+            // log warning if tasks didn't terminate
+        }
+    } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+    }
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 7

__

Why: Valid concern that shutdownNow() doesn't guarantee immediate termination, and ongoing rebalance tasks could access closed resources. Adding awaitTermination() is a good practice for clean shutdown, though the impact depends on timing and whether tasks are actually running during close.

Medium
Use reliable overflow detection

The overflow check sum < prev only detects overflow when adding positive values. If
any min value is Long.MAX_VALUE, the addition will overflow but the check may not
catch it correctly. Use Math.addExact() to reliably detect overflow or validate that
individual min values are reasonable before summing.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-194]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
-        long prev = sum;
-        sum += min;
-        if (sum < prev) {
+        try {
+            sum = Math.addExact(sum, min);
+        } catch (ArithmeticException e) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
     }
Suggestion importance[1-10]: 6

__

Why: The suggestion to use Math.addExact() is a minor improvement for overflow detection. The existing check works for positive values, but Math.addExact() is more explicit and handles edge cases better. This is a code quality improvement rather than a critical bug fix.

Low
Suggestions up to commit 50cf961
CategorySuggestion                                                                                                                                    Impact
Possible issue
Cancel rebalance task before shutdown

The close() method doesn't cancel the scheduled rebalance task before shutting down
the executor. If a rebalance is running when shutdownNow() is called, it may
continue accessing pools that are being closed, causing race conditions. Cancel
rebalanceTask before shutting down the executor.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
+    if (rebalanceTask != null) {
+        org.opensearch.common.util.concurrent.FutureUtils.cancel(rebalanceTask);
+        rebalanceTask = null;
+    }
     rebalancer.shutdownNow();
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 7

__

Why: Valid suggestion to prevent race conditions during shutdown. Canceling the rebalanceTask before shutdownNow() ensures the rebalance operation doesn't access pools being closed. However, this is a defensive improvement rather than a critical bug fix.

Medium
General
Validate non-negative pool minimums

The overflow check if (sum < prev) only detects overflow after it occurs. If min is
negative, the check may not detect the issue correctly. Add validation to ensure all
min values are non-negative before performing the sum.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-205]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
+        if (min < 0) {
+            throw new IllegalArgumentException("Pool minimum cannot be negative: " + min);
+        }
         long prev = sum;
         sum += min;
         if (sum < prev) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
     }
     if (sum > rootLimit) {
         throw new IllegalArgumentException(
             "Sum of pool minimums ("
                 + sum
                 + " bytes) exceeds root limit ("
                 + rootLimit
                 + " bytes). "
                 + "Reduce pool minimums or increase "
                 + NativeAllocatorPoolConfig.SETTING_ROOT_LIMIT
         );
     }
 }
Suggestion importance[1-10]: 6

__

Why: Adding validation for negative min values improves robustness. However, the settings framework already enforces non-negative values via Setting.longSetting(..., 0L, ...), making this a defensive check rather than addressing an actual vulnerability.

Low
Fail fast when allocator unavailable

The fallback RootAllocator with Long.MAX_VALUE limit is created without any bounds
checking. If the native allocator is unavailable, this could allow unbounded memory
allocation. Consider using a reasonable default limit or propagating the exception
to fail fast.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/memory/ArrowBufferPool.java [43-58]

 public ArrowBufferPool() {
     BufferAllocator alloc;
     boolean owns = false;
     try {
         alloc = ArrowNativeAllocator.instance().getPoolAllocator(NativeAllocatorPoolConfig.POOL_INGEST);
     } catch (IllegalStateException e) {
-        alloc = new RootAllocator(Long.MAX_VALUE);
-        owns = true;
-        logger.warn("NativeAllocator not available, using standalone RootAllocator");
+        logger.error("NativeAllocator not available, cannot create ArrowBufferPool", e);
+        throw new IllegalStateException("ArrowBufferPool requires NativeAllocator to be initialized", e);
     }
     this.poolAllocator = alloc;
     this.ownsAllocator = owns;
     long limit = poolAllocator.getLimit();
     this.maxChildAllocation = limit == Long.MAX_VALUE ? Long.MAX_VALUE : limit / 10;
     logger.debug("ArrowBufferPool: limit={}, maxChildAllocation={}", limit, maxChildAllocation);
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion to fail fast instead of falling back to unbounded allocation is reasonable for production code. However, the fallback provides resilience for testing/development scenarios. The impact depends on deployment context, making this a moderate improvement.

Low
Suggestions up to commit c4621a6
CategorySuggestion                                                                                                                                    Impact
Possible issue
Cancel rebalance task before shutdown

The close() method doesn't cancel the scheduled rebalance task before shutting down
the executor. If a rebalance is in progress, it could continue executing after
shutdownNow() is called, potentially accessing closed allocators. Cancel
rebalanceTask before shutting down the executor.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [172-184]

 @Override
 public void close() {
+    if (rebalanceTask != null) {
+        org.opensearch.common.util.concurrent.FutureUtils.cancel(rebalanceTask);
+        rebalanceTask = null;
+    }
     rebalancer.shutdownNow();
     pools.forEach((name, handle) -> {
         try {
             handle.allocator.close();
         } catch (Exception e) {
             // best-effort
         }
     });
     pools.clear();
     root.close();
     INSTANCE = null;
 }
Suggestion importance[1-10]: 7

__

Why: This is a valid concurrency concern. Canceling the rebalanceTask before shutting down the executor prevents potential race conditions where a rebalance operation might access closed allocators. This improves the robustness of the shutdown sequence.

Medium
Prevent partially constructed singleton exposure

The singleton INSTANCE is set before the allocator is fully initialized. If another
thread accesses instance() during construction, it could observe a partially
constructed object. Move the INSTANCE = this; assignment to the end of the
constructor after all fields are initialized.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowNativeAllocator.java [73-84]

+public ArrowNativeAllocator(long rootLimit) {
+    this.root = new RootAllocator(rootLimit);
+    org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor executor =
+        new org.opensearch.threadpool.Scheduler.SafeScheduledThreadPoolExecutor(1, r -> {
+            Thread t = new Thread(r, "native-allocator-rebalancer");
+            t.setDaemon(true);
+            return t;
+        });
+    executor.setRemoveOnCancelPolicy(true);
+    this.rebalancer = executor;
+    INSTANCE = this;
+}
 
-
Suggestion importance[1-10]: 3

__

Why: While the concern about singleton initialization is valid, the INSTANCE assignment is already at the end of the constructor after all fields are initialized. The 'improved_code' is identical to the 'existing_code', indicating no actual change is needed.

Low
General
Use Math.addExact for overflow detection

The overflow check if (sum < prev) only detects overflow after it occurs. For large
positive values, adding another positive value can wrap to a negative number, which
would still be less than prev but represents overflow. Use Math.addExact() to detect
overflow reliably.

plugins/arrow-base/src/main/java/org/opensearch/arrow/allocator/ArrowBasePlugin.java [182-205]

 private static void validateMinSum(long rootLimit, long... mins) {
     if (rootLimit == Long.MAX_VALUE) {
         return;
     }
     long sum = 0;
     for (long min : mins) {
-        long prev = sum;
-        sum += min;
-        if (sum < prev) {
+        try {
+            sum = Math.addExact(sum, min);
+        } catch (ArithmeticException e) {
             throw new IllegalArgumentException("Sum of pool minimums overflows.");
         }
     }
     if (sum > rootLimit) {
         throw new IllegalArgumentException(
             "Sum of pool minimums ("
                 + sum
                 + " bytes) exceeds root limit ("
                 + rootLimit
                 + " bytes). "
                 + "Reduce pool minimums or increase "
                 + NativeAllocatorPoolConfig.SETTING_ROOT_LIMIT
         );
     }
 }
Suggestion importance[1-10]: 6

__

Why: Using Math.addExact() is a more robust approach to overflow detection than manual comparison. The current check if (sum < prev) works for most cases but Math.addExact() provides clearer intent and handles all overflow scenarios consistently.

Low
Prevent allocator leak in constructor

The fallback RootAllocator created in the catch block is never closed if the
ArrowBufferPool is closed before being used. If an exception occurs after creating
the fallback allocator but before assignment, it will leak. Ensure proper cleanup in
exception paths.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/memory/ArrowBufferPool.java [43-58]

 public ArrowBufferPool() {
-    BufferAllocator alloc;
+    BufferAllocator alloc = null;
     boolean owns = false;
     try {
         alloc = ArrowNativeAllocator.instance().getPoolAllocator(NativeAllocatorPoolConfig.POOL_INGEST);
     } catch (IllegalStateException e) {
         alloc = new RootAllocator(Long.MAX_VALUE);
         owns = true;
         logger.warn("NativeAllocator not available, using standalone RootAllocator");
     }
     this.poolAllocator = alloc;
     this.ownsAllocator = owns;
     long limit = poolAllocator.getLimit();
     this.maxChildAllocation = limit == Long.MAX_VALUE ? Long.MAX_VALUE : limit / 10;
     logger.debug("ArrowBufferPool: limit={}, maxChildAllocation={}", limit, maxChildAllocation);
 }
Suggestion importance[1-10]: 2

__

Why: The 'improved_code' is nearly identical to the 'existing_code' (only changing BufferAllocator alloc; to BufferAllocator alloc = null;). The concern about leaks is theoretical since no exception can occur between allocator creation and assignment in this simple constructor. The suggestion provides minimal value.

Low

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from d9563d1 to 2ab615d Compare May 17, 2026 13:25
@Bukhtawar Bukhtawar changed the title Add unified native allocator framework to unify Arrow allocators Add unified native allocator framework to unify Arrow allocations May 17, 2026
@Bukhtawar Bukhtawar changed the title Add unified native allocator framework to unify Arrow allocations Add unified native allocator framework to track and monitor Arrow allocations May 17, 2026
@Bukhtawar Bukhtawar changed the title Add unified native allocator framework to track and monitor Arrow allocations Unified native allocator framework to track and monitor Arrow allocations May 17, 2026
@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 0bdd287 to 15f8778 Compare May 17, 2026 13:31
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 15f8778

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8e8c891

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 8e8c891 to 8a73585 Compare May 17, 2026 13:49
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8a73585

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 8a73585 to a7338f8 Compare May 17, 2026 13:53
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a7338f8

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c39f543

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for c39f543: 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?

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from c39f543 to 5d8de32 Compare May 17, 2026 14:48
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b3f6bf8

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for b3f6bf8: 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?

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from b3f6bf8 to 169dad2 Compare May 17, 2026 15:52
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 169dad2

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 169dad2 to 4ae2694 Compare May 17, 2026 16:08
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4ae2694

@Bukhtawar
Bukhtawar requested a review from sohami as a code owner May 19, 2026 01:14
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 240fc89

@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 240fc89 to 8d592a4 Compare May 19, 2026 01:23
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8d592a4

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
@Bukhtawar
Bukhtawar force-pushed the unified-arrow-allocator branch from 8d592a4 to 481de66 Compare May 19, 2026 01:29
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 481de66

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 481de66: SUCCESS

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 670cb36

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 670cb36: 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?

gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… listener SPI, plugin nodeStats

Builds on Bukhtawar's unified arrow-base framework (opensearch-project#21703) to:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max).
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, analytics-engine + datafusion
  to the QUERY/DATAFUSION pools. Hard-fail if the framework plugin is
  missing - silently skipping the wire-up is the silent-misconfiguration
  class of bug Phase 1 set out to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Plug in NativeAllocatorListener SPI so DataFusionPlugin can mirror the
  datafusion-pool max into the Rust-side MemoryPool via df_set_memory_pool_limit.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats; removes the dedicated
  _native_allocator/stats REST endpoint (6 stats classes + 1 test deleted).

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the pool, those ITs fail at node startup
  with "ArrowNativeAllocator not initialized". Each IT now installs the
  framework plugin and lists it as an extendedPlugin.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with consistent semantics.
@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 670cb36: 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?

gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… listener SPI, plugin nodeStats

Builds on Bukhtawar's unified arrow-base framework (opensearch-project#21703) to:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max).
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, analytics-engine + datafusion
  to the QUERY/DATAFUSION pools. Hard-fail if the framework plugin is
  missing - silently skipping the wire-up is the silent-misconfiguration
  class of bug Phase 1 set out to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Plug in NativeAllocatorListener SPI so DataFusionPlugin can mirror the
  datafusion-pool max into the Rust-side MemoryPool via df_set_memory_pool_limit.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats; removes the dedicated
  _native_allocator/stats REST endpoint (6 stats classes + 1 test deleted).

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the pool, those ITs fail at node startup
  with "ArrowNativeAllocator not initialized". Each IT now installs the
  framework plugin and lists it as an extendedPlugin.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with consistent semantics.
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… listener SPI, plugin nodeStats

Builds on Bukhtawar's unified arrow-base framework (opensearch-project#21703) to:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max).
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, analytics-engine + datafusion
  to the QUERY/DATAFUSION pools. Hard-fail if the framework plugin is
  missing - silently skipping the wire-up is the silent-misconfiguration
  class of bug Phase 1 set out to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Plug in NativeAllocatorListener SPI so DataFusionPlugin can mirror the
  datafusion-pool max into the Rust-side MemoryPool via df_set_memory_pool_limit.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats; removes the dedicated
  _native_allocator/stats REST endpoint (6 stats classes + 1 test deleted).

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the pool, those ITs fail at node startup
  with "ArrowNativeAllocator not initialized". Each IT now installs the
  framework plugin and lists it as an extendedPlugin.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with consistent semantics.
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… listener SPI, plugin nodeStats

Builds on Bukhtawar's unified arrow-base framework (opensearch-project#21703) to:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max).
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, analytics-engine + datafusion
  to the QUERY/DATAFUSION pools. Hard-fail if the framework plugin is
  missing - silently skipping the wire-up is the silent-misconfiguration
  class of bug Phase 1 set out to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Plug in NativeAllocatorListener SPI so DataFusionPlugin can mirror the
  datafusion-pool max into the Rust-side MemoryPool via df_set_memory_pool_limit.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats; removes the dedicated
  _native_allocator/stats REST endpoint (6 stats classes + 1 test deleted).

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the pool, those ITs fail at node startup
  with "ArrowNativeAllocator not initialized". Each IT now installs the
  framework plugin and lists it as an extendedPlugin.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with consistent semantics.
@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 670cb36: SUCCESS

@Bukhtawar
Bukhtawar merged commit ac2b2fd into opensearch-project:main May 19, 2026
34 of 43 checks passed
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… listener SPI, plugin nodeStats

Builds on Bukhtawar's unified arrow-base framework (opensearch-project#21703) to:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max).
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, analytics-engine + datafusion
  to the QUERY/DATAFUSION pools. Hard-fail if the framework plugin is
  missing - silently skipping the wire-up is the silent-misconfiguration
  class of bug Phase 1 set out to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Plug in NativeAllocatorListener SPI so DataFusionPlugin can mirror the
  datafusion-pool max into the Rust-side MemoryPool via df_set_memory_pool_limit.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats; removes the dedicated
  _native_allocator/stats REST endpoint (6 stats classes + 1 test deleted).

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the pool, those ITs fail at node startup
  with "ArrowNativeAllocator not initialized". Each IT now installs the
  framework plugin and lists it as an extendedPlugin.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with consistent semantics.
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
… plugin nodeStats

Builds on opensearch-project#21703 to add the pieces a real production deployment needs:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max). FLIGHT_MIN/INGEST_MIN defaults are 0L (the prior
  Long.MAX_VALUE defaults caused the new validator to reject any non-MAX
  root). setPoolMin now updates the live BufferAllocator.setLimit so the
  Dynamic property has observable effect even when the rebalancer is off.
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY and DATAFUSION pools alongside FLIGHT and INGEST. Pools init
  at min when the rebalancer is enabled; otherwise at max so non-rebalanced
  nodes can still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, parquet-data-format to the
  INGEST pool, and analytics-engine via the framework's allocator service.
  Hard-fail if the framework plugin is missing — silently skipping the
  wire-up is the silent-misconfiguration class of bug we want to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Rebalancer now distributes headroom across all pools (not only those with
  current allocation > 0). Avoids the dead-pool corner case where a pool
  with min=0 starts at limit=0, can never make a first allocation, and
  never receives a bonus.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* Each entry is wire-framed as (name, length-prefixed bytes); the receiver
  wraps the inner payload with NamedWriteableAwareStreamInput and drops
  entries whose subtype is not registered locally. This makes mixed-version
  rolling upgrades safe — a coordinator that lacks the plugin a data node
  is running keeps decoding the rest of NodeStats instead of failing the
  whole response.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats. The dedicated _native_allocator/stats
  REST endpoint is gone — one observability surface, not two.
* Plugin stats are emitted on every _nodes/stats request regardless of the
  ?metric= filter; matches RemoteStoreNodeStats precedent. A future PR can
  add a Metric.PLUGIN_STATS gate without breaking the wire protocol.

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with a hard upper
  bound of 100 to reject fat-finger PUTs that would starve every VSR.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the FLIGHT pool, those ITs fail at node
  startup with "ArrowNativeAllocator not initialized". Each IT now installs
  the framework plugin and lists it as an extendedPlugin.

Signed-off-by: Gaurav Singh <snghsvn@amazon.com>
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
…Stats

Builds on opensearch-project#21703 to add the pieces a real production deployment needs:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max). FLIGHT_MIN/INGEST_MIN defaults are 0L (the prior
  Long.MAX_VALUE defaults caused the new validator to reject any non-MAX
  root). setPoolMin now updates the live BufferAllocator.setLimit so the
  Dynamic property has observable effect even when the rebalancer is off.
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY pool alongside FLIGHT and INGEST. Pools init at min when the
  rebalancer is enabled; otherwise at max so non-rebalanced nodes can
  still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, parquet-data-format to the
  INGEST pool, and analytics-engine to the QUERY pool via the framework's
  allocator service. Hard-fail if the framework plugin is missing —
  silently skipping the wire-up is the silent-misconfiguration class of
  bug we want to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Rebalancer now distributes headroom across all pools (not only those with
  current allocation > 0). Avoids the dead-pool corner case where a pool
  with min=0 starts at limit=0, can never make a first allocation, and
  never receives a bonus.

DataFusion runtime memory accounting stays separate. The Rust-side
DataFusion MemoryPool is governed by datafusion.memory_pool_limit_bytes
(unchanged from before), which is the right knob: DataFusion's internal
sort/hash/group-by working memory is allocated by Rust, not through the
Arrow Java BufferAllocator hierarchy, so a Java-side pool would resize a
ceiling no allocator routes through. The framework's QUERY pool covers
the cross-plugin Arrow allocations the analytics-engine plumbing makes,
which is what we want bounded centrally.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* Each entry is wire-framed as (name, length-prefixed bytes); the receiver
  wraps the inner payload with NamedWriteableAwareStreamInput and drops
  entries whose subtype is not registered locally. This makes mixed-version
  rolling upgrades safe — a coordinator that lacks the plugin a data node
  is running keeps decoding the rest of NodeStats instead of failing the
  whole response.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats. The dedicated _native_allocator/stats
  REST endpoint is gone — one observability surface, not two.
* Plugin stats are emitted on every _nodes/stats request regardless of the
  ?metric= filter; matches RemoteStoreNodeStats precedent.

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with a hard upper
  bound of 100 to reject fat-finger PUTs that would starve every VSR.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the FLIGHT pool, those ITs fail at node
  startup with "ArrowNativeAllocator not initialized". Each IT now installs
  the framework plugin and lists it as an extendedPlugin.

Signed-off-by: Gaurav Singh <snghsvn@amazon.com>
gaurav-amz added a commit to gaurav-amz/OpenSearch that referenced this pull request May 19, 2026
…Stats

Builds on opensearch-project#21703 to add the pieces a real production deployment needs:

* Make pool min/max settings dynamic and grouped-validate cross-setting
  invariants on every cluster-state update (sum of pool mins <= root,
  per-pool min <= max). FLIGHT_MIN/INGEST_MIN defaults are 0L (the prior
  Long.MAX_VALUE defaults caused the new validator to reject any non-MAX
  root). setPoolMin now updates the live BufferAllocator.setLimit so the
  Dynamic property has observable effect even when the rebalancer is off.
* Derive ROOT_LIMIT_SETTING default from the AC node-native-memory budget
  (limit minus buffer-percent) so the framework respects the same budget
  AC throttles on, with a Long.MAX_VALUE fallback when AC is unconfigured.
* Add QUERY pool alongside FLIGHT and INGEST. Pools init at min when the
  rebalancer is enabled; otherwise at max so non-rebalanced nodes can
  still allocate.
* Wire arrow-flight-rpc to the FLIGHT pool, parquet-data-format to the
  INGEST pool, and analytics-engine to the QUERY pool via the framework's
  allocator service. Hard-fail if the framework plugin is missing —
  silently skipping the wire-up is the silent-misconfiguration class of
  bug we want to prevent.
* Cleanup ad-hoc allocator fallbacks in parquet-data-format / analytics-engine
  so all Arrow consumers go through the unified pool hierarchy.
* Rebalancer now distributes headroom across all pools (not only those with
  current allocation > 0). Avoids the dead-pool corner case where a pool
  with min=0 starts at limit=0, can never make a first allocation, and
  never receives a bonus.

DataFusion runtime memory accounting stays separate. The Rust-side
DataFusion MemoryPool is governed by datafusion.memory_pool_limit_bytes
(unchanged from before), which is the right knob: DataFusion's internal
sort/hash/group-by working memory is allocated by Rust, not through the
Arrow Java BufferAllocator hierarchy, so a Java-side pool would resize a
ceiling no allocator routes through. The framework's QUERY pool covers
the cross-plugin Arrow allocations the analytics-engine plumbing makes,
which is what we want bounded centrally.

Plugin _nodes/stats integration:
* Add Plugin#nodeStats() hook + PluginNodeStats interface in server.
* Wire NodeStats to carry Map<String, PluginNodeStats> with version-gated
  ser/deser at V_3_7_0 and top-level rendering under nodes.<id>.<name>.
* Each entry is wire-framed as (name, length-prefixed bytes); the receiver
  wraps the inner payload with NamedWriteableAwareStreamInput and drops
  entries whose subtype is not registered locally. This makes mixed-version
  rolling upgrades safe — a coordinator that lacks the plugin a data node
  is running keeps decoding the rest of NodeStats instead of failing the
  whole response.
* NativeAllocatorPluginStats adapter wraps NativeAllocatorPoolStats so the
  framework contributes to _nodes/stats. The dedicated _native_allocator/stats
  REST endpoint is gone — one observability surface, not two.
* Plugin stats are emitted on every _nodes/stats request regardless of the
  ?metric= filter; matches RemoteStoreNodeStats precedent.

DataFusion spill memory limit:
* Promote datafusion.spill_memory_limit_bytes to Setting.Property.Dynamic.
* Wire addSettingsUpdateConsumer that branches on
  NativeBridge.isSpillLimitDynamic(): mirrors live when df_set_spill_limit
  is exported, otherwise warns and waits for next node restart.

API hygiene:
* parquet.max_per_vsr_allocation_ratio is a divisor (limit/N), not a ratio.
  Renamed to parquet.max_per_vsr_allocation_divisor with a hard upper
  bound of 100 to reject fat-finger PUTs that would starve every VSR.

Real regressions caught during self-audit:
* Existing arrow-flight-rpc internalClusterTests and the sandbox coordinator
  ITs were not declaring the framework plugin in nodePlugins(). After the
  flight transport got wired to the FLIGHT pool, those ITs fail at node
  startup with "ArrowNativeAllocator not initialized". Each IT now installs
  the framework plugin and lists it as an extendedPlugin.

Signed-off-by: Gaurav Singh <snghsvn@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants