Add native memory stat and task cancellation scaffolding by AjayRajNelapudi · Pull Request #21637 · opensearch-project/OpenSearch

AjayRajNelapudi · 2026-05-13T11:03:53Z

Description

Add native memory stat and task cancellation scaffolding

% curl -XGET 'localhost:9200/_nodes/stats/native_memory?pretty'
{
  "_nodes" : {
    "total" : 1,
    "successful" : 1,
    "failed" : 0
  },
  "cluster_name" : "runTask",
  "nodes" : {
    "S8n7MR7-SfuUDMLJTqVaJA" : {
      "timestamp" : 1778669796651,
      "name" : "runTask-0",
      "transport_address" : "11.112.218.66:9300",
      "host" : "11.112.218.66",
      "ip" : "11.112.218.66:9300",
      "roles" : [
        "cluster_manager",
        "data",
        "ingest",
        "remote_cluster_client"
      ],
      "attributes" : {
        "testattr" : "test",
        "shard_indexing_pressure_enabled" : "true"
      },
      "native_memory" : {
        "allocated_bytes" : 3908928,
        "resident_bytes" : 10387456
      }
    }
  }
}

% curl -XGET 'localhost:9200/_nodes/stats/task_cancellation?pretty'
{
  "_nodes" : {
    "total" : 1,
    "successful" : 1,
    "failed" : 0
  },
  "cluster_name" : "runTask",
  "nodes" : {
    "Yc2tKXPcSLaLXFN1QkS2sg" : {
      "timestamp" : 1778670179233,
      "name" : "runTask-0",
      "transport_address" : "11.112.218.66:9300",
      "host" : "11.112.218.66",
      "ip" : "11.112.218.66:9300",
      "roles" : [
        "cluster_manager",
        "data",
        "ingest",
        "remote_cluster_client"
      ],
      "attributes" : {
        "testattr" : "test",
        "shard_indexing_pressure_enabled" : "true"
      },
      "task_cancellation" : {
        "search_task" : {
          "current_count_post_cancel" : 0,
          "total_count_post_cancel" : 0
        },
        "search_shard_task" : {
          "current_count_post_cancel" : 0,
          "total_count_post_cancel" : 0
        },
        "native_search_task" : {
          "current_count_post_cancel" : 0,
          "total_count_post_cancel" : 0
        },
        "native_search_shard_task" : {
          "current_count_post_cancel" : 0,
          "total_count_post_cancel" : 0
        }
      }
    }
  }
}

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.

himshikhagupta

Code changes look good. Can we just check the structuring once?

github-actions · 2026-05-14T03:16:26Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 963e148.

Path	Line	Severity	Description
sandbox/libs/dataformat-native/rust/Cargo.toml	75	high	New external Rust dependency added: proptest = "=1.4.0". Per mandatory rule, all dependency additions must be flagged regardless of apparent legitimacy. Maintainers should verify this artifact's provenance and integrity in crates.io.
sandbox/plugins/analytics-backend-datafusion/rust/Cargo.toml	85	high	New Rust dev-dependency added via workspace: proptest = { workspace = true }. Per mandatory rule, all dependency changes must be flagged. Maintainers should confirm the workspace-level proptest version and source are expected.
sandbox/libs/dataformat-native/licenses/log4j-api-2.25.4.jar.sha1	1	medium	SHA1 checksum file for log4j-api-2.25.4.jar is deleted while the dependency itself remains active in build.gradle. Removing integrity verification metadata for an active dependency is unusual and warrants confirmation that the artifact is still being verified elsewhere in the build pipeline.
sandbox/libs/dataformat-native/build.gradle	16	low	A new compileOnly dependency on the ':server' module is introduced from a sandbox library. This creates an unusual dependency inversion (sandbox lib depending on server), which could broaden the attack surface available to sandbox code at compile time. Should be reviewed for architectural intent.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 2 | Medium: 1 | Low: 1

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.

github-actions · 2026-05-16T00:55:18Z

PR Reviewer Guide 🔍

(Review updated until commit `4e4afe2`)

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 `fetchNativeStats()` method catches all exceptions and logs them at debug level, then returns null. If the supplier throws an exception during normal operation (e.g., due to a transient native library issue), the error is silently suppressed. This could mask real problems in production. Consider logging at warn level or propagating the exception to the caller so that monitoring systems can detect failures in native stats collection. @Nullable private AnalyticsBackendTaskCancellationStats fetchNativeStats() { if (nativeStatsSupplier == null) { return null; } try { return nativeStatsSupplier.get(); } catch (Exception e) { logger.debug("Failed to fetch native task cancellation stats", e); return null; } } Possible Issue The `getAnalyticsBackendTaskCancellationStats()` method catches all exceptions and returns a zero-filled stats object. This makes it impossible to distinguish between "no tasks cancelled" and "stats collection failed". Callers cannot detect when the native bridge is broken. Consider returning null on error (matching the pattern used in `getAnalyticsBackendNativeMemoryStats()`) or using a sentinel value like -1 for all fields to indicate error state. @Override public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Possible Issue The `stats()` method is synchronized, which could become a bottleneck if called frequently (e.g., on every `_nodes/stats` request under high load). The synchronization is only needed to protect the cache refresh logic. Consider using a lock-free approach (e.g., AtomicLong for lastRefreshTimestamp and volatile for cachedStats) or a read-write lock to allow concurrent reads when the cache is valid. public synchronized AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); lastRefreshTimestamp = System.currentTimeMillis(); } return cachedStats; } Possible Issue The constructor parameter list has 28 parameters after adding `nativeMemoryStats`. This is extremely difficult to maintain and error-prone. While not a bug in this PR, adding another parameter to an already overloaded constructor increases the risk of parameter order mistakes in future changes. Consider using a builder pattern or parameter object for NodeStats construction. public NodeStats( DiscoveryNode node, long timestamp, @Nullable NodeIndicesStats indices, @Nullable OsStats os, @Nullable ProcessStats process, @Nullable JvmStats jvm, @Nullable ThreadPoolStats threadPool, @Nullable FsInfo fs, @Nullable TransportStats transport, @Nullable HttpStats http, @Nullable AllCircuitBreakerStats breaker, @Nullable ScriptStats scriptStats, @Nullable DiscoveryStats discoveryStats, @Nullable IngestStats ingestStats, @Nullable AdaptiveSelectionStats adaptiveSelectionStats, @Nullable NodesResourceUsageStats resourceUsageStats, @Nullable ScriptCacheStats scriptCacheStats, @Nullable IndexingPressureStats indexingPressureStats, @Nullable ShardIndexingPressureStats shardIndexingPressureStats, @Nullable SearchBackpressureStats searchBackpressureStats, @Nullable ClusterManagerThrottlingStats clusterManagerThrottlingStats, @Nullable WeightedRoutingStats weightedRoutingStats, @Nullable AggregateFileCacheStats fileCacheStats, @Nullable TaskCancellationStats taskCancellationStats, @Nullable SearchPipelineStats searchPipelineStats, @Nullable SegmentReplicationRejectionStats segmentReplicationRejectionStats, @Nullable RepositoriesStats repositoriesStats, @Nullable AdmissionControlStats admissionControlStats, @Nullable NodeCacheStats nodeCacheStats, @Nullable RemoteStoreNodeStats remoteStoreNodeStats, @Nullable AnalyticsBackendNativeMemoryStats nativeMemoryStats ) {

github-actions · 2026-05-16T00:56:03Z

PR Code Suggestions ✨

Latest suggestions up to 4e4afe2

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Log exception before returning fallback The exception handler silently swallows all exceptions and returns zero values. This masks critical errors like native library initialization failures. Consider logging the exception at WARN level before returning the fallback values to aid debugging. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that exceptions are silently swallowed when fetching native task cancellation stats. Adding logging would improve debuggability. However, the impact is moderate since the fallback behavior (returning zeros) is reasonable for stats collection.	Medium
	Log exception before returning error sentinel The exception handler silently swallows all exceptions and returns error sentinel values. This masks critical errors like native library initialization failures. Consider logging the exception at WARN level before returning the fallback values to aid debugging. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [297-305] public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 7 __ Why: Similar to the first suggestion, this correctly identifies that exceptions are silently swallowed when fetching native memory stats. Adding logging would improve debuggability. The impact is moderate since the fallback behavior (returning -1 sentinel values) is reasonable for stats collection.	Medium
	Replace synchronized with lock-free atomics The synchronized method could become a bottleneck under high concurrent access. Consider using a lock-free approach with `AtomicReference` and `compareAndSet` for the cache update, or use a `ReentrantReadWriteLock` to allow concurrent reads while serializing writes. server/src/main/java/org/opensearch/monitor/memory/NativeMemoryService.java [73-83] -public synchronized AnalyticsBackendNativeMemoryStats stats() { +private final AtomicReference<AnalyticsBackendNativeMemoryStats> cachedStatsRef = new AtomicReference<>(); +private final AtomicLong lastRefreshTimestampRef = new AtomicLong(0); + +public AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } - if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { + long now = System.currentTimeMillis(); + if ((now - lastRefreshTimestampRef.get()) > refreshInterval.millis()) { AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); - cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); - lastRefreshTimestamp = System.currentTimeMillis(); + AnalyticsBackendNativeMemoryStats newStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); + cachedStatsRef.set(newStats); + lastRefreshTimestampRef.set(now); } - return cachedStats; + return cachedStatsRef.get(); } Suggestion importance[1-10]: 4 __ Why: While the suggestion to use lock-free atomics could improve concurrency, the current `synchronized` method is unlikely to be a bottleneck for stats collection (typically called once per refresh interval). The suggested implementation also has a race condition where multiple threads could call `statsSupplier.get()` concurrently during cache expiry, potentially causing unnecessary work.	Low

Previous suggestions

Suggestions up to commit 4e4afe2

Category	Suggestion	Impact
General	Use consistent timestamp for cache logic The method uses `System.currentTimeMillis()` twice, which could return different values if called at different times. Store the current time in a local variable at the start of the method to ensure consistent time-based logic and avoid potential race conditions. server/src/main/java/org/opensearch/monitor/memory/NativeMemoryService.java [73-83] public synchronized AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } - if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { + long currentTime = System.currentTimeMillis(); + if ((currentTime - lastRefreshTimestamp) > refreshInterval.millis()) { AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); - lastRefreshTimestamp = System.currentTimeMillis(); + lastRefreshTimestamp = currentTime; } return cachedStats; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential inconsistency where `System.currentTimeMillis()` is called twice. While the method is synchronized (preventing race conditions), using a single timestamp improves code clarity and ensures the same time value is used for both the comparison and the assignment to `lastRefreshTimestamp`.	Medium
	Log exception before returning fallback The catch block silently swallows all exceptions and returns zero values, which makes debugging native bridge failures difficult. Consider logging the exception at WARN or ERROR level before returning the fallback values to aid troubleshooting. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] @Override public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that exceptions are silently swallowed, which can hinder debugging. However, the impact is moderate since the method already returns fallback values that indicate failure. Adding logging would improve observability but is not critical for correctness.	Low
	Log exception before returning error state The catch block silently swallows all exceptions and returns error sentinel values (-1, -1), which makes debugging native memory fetcher failures difficult. Consider logging the exception at WARN or ERROR level before returning the error state to aid troubleshooting. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [297-305] @Override public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 6 __ Why: Similar to the first suggestion, this correctly identifies silent exception swallowing. The error state (-1, -1) already signals failure, but logging would aid troubleshooting. The impact is moderate since the error handling contract is preserved.	Low

Suggestions up to commit 298b746

Category	Suggestion	Impact
General	Log native stats fetch failures The exception handler swallows all exceptions silently, returning zero values that are indistinguishable from legitimate zero counters. This masks failures in the native bridge. Add logging to record when the native call fails, so operators can detect and diagnose issues with the native stats collection. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that exceptions are swallowed silently when fetching native task cancellation stats. Adding logging would help operators diagnose issues. However, the impact is moderate since the method already returns a safe fallback value.	Medium
	Log native memory fetch failures The exception handler swallows all exceptions silently, returning error sentinel values (-1, -1) without logging. This makes it difficult to diagnose why native memory stats are unavailable. Add logging to record when the fetch fails, helping operators identify configuration or runtime issues. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [296-305] public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 7 __ Why: Similar to the first suggestion, this correctly identifies silent exception handling in the native memory stats supplier. Adding logging would improve observability. The impact is moderate as the error sentinel values (-1, -1) are already returned.	Medium
	Reduce synchronization contention with double-check The method uses `synchronized` on the entire method, which can cause contention when multiple threads request stats simultaneously. Consider using a more granular locking strategy or a lock-free approach with atomic operations to reduce contention, especially since the refresh check is a simple timestamp comparison. server/src/main/java/org/opensearch/monitor/memory/NativeMemoryService.java [73-83] -public synchronized AnalyticsBackendNativeMemoryStats stats() { +public AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } - if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { - AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); - cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); - lastRefreshTimestamp = System.currentTimeMillis(); + long now = System.currentTimeMillis(); + if ((now - lastRefreshTimestamp) > refreshInterval.millis()) { + synchronized (this) { + if ((now - lastRefreshTimestamp) > refreshInterval.millis()) { + AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); + cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); + lastRefreshTimestamp = now; + } + } } return cachedStats; } Suggestion importance[1-10]: 6 __ Why: The suggestion proposes a double-checked locking pattern to reduce contention. While this could improve performance under high concurrency, the current `synchronized` method is simpler and less error-prone. The performance gain may be marginal given the 1-second default refresh interval, and the added complexity may not be justified.	Low

Suggestions up to commit 920793a

Category	Suggestion	Impact
General	Log exception when stats fetch fails The catch block silently swallows all exceptions and returns zero stats. This masks critical errors like native library initialization failures. Consider logging the exception at WARN or ERROR level to aid debugging when native stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that exceptions are silently swallowed. However, the `NativeMemoryFetcher.fetch()` already logs warnings on error (line 56 in `NativeMemoryFetcher.java`), so adding another log here would be redundant. The score reflects that while the concern is valid, the implementation already handles logging at a lower level.	Medium
	Log exception when memory stats fetch fails The catch block silently swallows all exceptions and returns error sentinel values. This masks critical errors like native library initialization failures. Consider logging the exception at WARN or ERROR level to aid debugging when native memory stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [296-305] public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 7 __ Why: Similar to the previous suggestion, the concern about silent exception handling is valid. However, `NativeMemoryFetcher.fetch()` already logs warnings (line 56), making additional logging here redundant. The score reflects that the suggestion addresses a real concern but the implementation already provides the desired logging behavior.	Medium
	Capture timestamp once for consistency The method uses `synchronized` for thread safety but calls `System.currentTimeMillis()` twice, which could produce inconsistent timestamps if the clock changes between calls. Capture the current time once at the start of the method to ensure consistent cache expiry logic. server/src/main/java/org/opensearch/monitor/memory/NativeMemoryService.java [73-83] public synchronized AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } - if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { + long currentTime = System.currentTimeMillis(); + if ((currentTime - lastRefreshTimestamp) > refreshInterval.millis()) { AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); - lastRefreshTimestamp = System.currentTimeMillis(); + lastRefreshTimestamp = currentTime; } return cachedStats; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a minor inefficiency where `System.currentTimeMillis()` is called twice. While capturing the timestamp once improves consistency and is a best practice, the practical impact is minimal since the method is `synchronized` and clock changes between the two calls are extremely unlikely. The improvement is valid but offers only marginal benefit.	Low

Suggestions up to commit e5fc074

Category	Suggestion	Impact
General	Log exception before returning fallback stats The exception handler swallows all exceptions silently and returns zero stats, making debugging difficult. Consider logging the exception at WARN or ERROR level before returning the fallback stats to aid troubleshooting when native stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that swallowing exceptions silently makes debugging difficult. Adding logging would improve observability. However, the `NativeMemoryFetcher.fetch()` already logs warnings on error (line 56 in `NativeMemoryFetcher.java`), so this is a minor improvement rather than a critical fix.	Medium
	Log exception before returning error state The exception handler swallows all exceptions silently and returns error sentinel values (-1, -1), making debugging difficult. Consider logging the exception at WARN or ERROR level before returning the error state to aid troubleshooting when native memory stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [296-305] public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 7 __ Why: Similar to the previous suggestion, adding logging would improve observability when native memory stats collection fails. The `NativeMemoryFetcher.fetch()` already logs warnings (line 56), but logging at the plugin level would provide additional context about where the failure occurred.	Medium
	Warn when multiple stats suppliers detected If multiple `SearchBackEndPlugin` instances provide native memory stats suppliers, only the first non-null supplier is registered. This could lead to unexpected behavior if multiple plugins compete. Consider logging a warning when multiple suppliers are detected to alert operators of potential configuration issues. server/src/main/java/org/opensearch/node/Node.java [1226-1232] -pluginsService.filterPlugins(SearchBackEndPlugin.class) +List<Supplier<AnalyticsBackendNativeMemoryStats>> suppliers = pluginsService.filterPlugins(SearchBackEndPlugin.class) .stream() .map(SearchBackEndPlugin::getAnalyticsBackendNativeMemoryStats) .filter(Objects::nonNull) - .findFirst() - .ifPresent(supplier -> monitorService.memoryReportingService().setNativeStatsSupplier(supplier)); + .collect(Collectors.toList()); +if (suppliers.size() > 1) { + logger.warn("Multiple SearchBackEndPlugin instances provide native memory stats suppliers; using first one"); +} +suppliers.stream().findFirst().ifPresent(supplier -> monitorService.memoryReportingService().setNativeStatsSupplier(supplier)); Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a potential configuration issue where multiple plugins might provide stats suppliers. While logging a warning would be helpful for operators, this scenario is unlikely in practice since typically only one analytics backend plugin would be active. The suggestion is valid but addresses a low-probability edge case.	Low

Suggestions up to commit e5fc074

Category	Suggestion	Impact
General	Log exception when stats fetch fails The catch block silently swallows all exceptions and returns zero stats. This masks critical errors like native library initialization failures. Consider logging the exception at WARN or ERROR level to aid debugging when native stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [286-294] public Supplier<AnalyticsBackendTaskCancellationStats> getAnalyticsBackendTaskCancellationStats() { return () -> { try { return NativeBridge.nativeNodeStats(); } catch (Exception e) { + logger.warn("Failed to fetch native task cancellation stats", e); return new AnalyticsBackendTaskCancellationStats(0, 0, 0, 0); } }; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the catch block silently swallows exceptions. Adding logging would improve debuggability when native stats collection fails. However, the impact is moderate since the method already returns a safe fallback value.	Medium
	Log exception when memory stats fetch fails The catch block silently swallows all exceptions and returns error sentinel values. This masks critical errors like FFM downcall failures. Consider logging the exception at WARN or ERROR level to aid debugging when native memory stats collection fails. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionPlugin.java [297-305] public Supplier<AnalyticsBackendNativeMemoryStats> getAnalyticsBackendNativeMemoryStats() { return () -> { try { return NativeMemoryFetcher.fetch(); } catch (Exception e) { + logger.warn("Failed to fetch native memory stats", e); return new AnalyticsBackendNativeMemoryStats(-1, -1); } }; } Suggestion importance[1-10]: 7 __ Why: Similar to the previous suggestion, this correctly identifies silent exception swallowing. Adding logging would aid debugging. The impact is moderate since the method returns error sentinel values (`-1, -1`) as a fallback.	Medium
	Use explicit lock for better concurrency The method uses `synchronized` on the entire method, which can cause contention when multiple threads call `stats()` concurrently. Consider using a more granular locking strategy or `ReentrantLock` to minimize the critical section, especially since the cache check is read-heavy. server/src/main/java/org/opensearch/monitor/memory/NativeMemoryService.java [73-83] -public synchronized AnalyticsBackendNativeMemoryStats stats() { +private final ReentrantLock lock = new ReentrantLock(); + +public AnalyticsBackendNativeMemoryStats stats() { if (statsSupplier == null) { return null; } - if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { - AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); - cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); - lastRefreshTimestamp = System.currentTimeMillis(); + lock.lock(); + try { + if ((System.currentTimeMillis() - lastRefreshTimestamp) > refreshInterval.millis()) { + AnalyticsBackendNativeMemoryStats fresh = statsSupplier.get(); + cachedStats = fresh != null ? fresh : new AnalyticsBackendNativeMemoryStats(-1, -1); + lastRefreshTimestamp = System.currentTimeMillis(); + } + return cachedStats; + } finally { + lock.unlock(); } - return cachedStats; } Suggestion importance[1-10]: 4 __ Why: The suggestion proposes replacing `synchronized` with `ReentrantLock` for better concurrency. However, the improvement is marginal since the critical section is already small and the method is called infrequently (once per refresh interval). The added complexity of explicit lock management is not justified by the minimal performance gain.	Low

github-actions · 2026-05-16T01:20:00Z

❌ Gradle check result for 963e148: 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-17T04:49:57Z

Persistent review updated to latest commit ce41123

github-actions · 2026-05-17T04:59:11Z

❌ Gradle check result for ce41123: 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-17T05:17:55Z

Persistent review updated to latest commit cf117b6

github-actions · 2026-05-17T05:25:12Z

❌ Gradle check result for cf117b6: 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-17T05:28:42Z

Persistent review updated to latest commit 20c1442

github-actions · 2026-05-17T05:33:57Z

❌ Gradle check result for 20c1442: 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-20T13:51:11Z

Persistent review updated to latest commit 920793a

Signed-off-by: Ajay Raj Nelapudi <ajnelapu@amazon.com>

github-actions · 2026-05-20T14:15:19Z

Persistent review updated to latest commit 298b746

Signed-off-by: Ajay Raj Nelapudi <ajnelapu@amazon.com>

github-actions · 2026-05-20T14:27:47Z

Persistent review updated to latest commit 4e4afe2

github-actions · 2026-05-20T14:51:35Z

Persistent review updated to latest commit 4e4afe2

github-actions · 2026-05-20T15:48:32Z

✅ Gradle check result for 4e4afe2: SUCCESS

The makeNodeStatsWithResourceUsage helper was missing the AnalyticsBackendNativeMemoryStats parameter added in opensearch-project#21637. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Fix StringViewArray buffer bloat: gc() before FFI export StringViewArray::slice() shares ALL backing buffers via Arc::clone. When DataFusion's hash aggregate emits output (EmitTo::All + slice into 8192-row batches), each slice carries the full backing buffer pool. For a 14M-group aggregate with SearchPhrase strings, this means 174MB per batch instead of 0.4MB — a 435x amplification. Add compact_string_view_columns() in stream_next() that calls gc() on Utf8View/BinaryView columns before C Data Interface export. This compacts each batch to contain only its own referenced strings. Validated on 4-shard ClickBench q19: Before: 9748 batches × 174MB = 1.7TB, 12 minutes After: 9748 batches × 0.4MB = 4GB, 11 seconds (65x faster) Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Add Rust unit tests for compact_string_view_columns Regression guard for e2fd9bc (StringView buffer bloat fix). Tests prove that sliced StringView/BinaryView batches carry inflated backing buffers and that gc() compacts them to proportional size. Covers: large buffer compaction, inline-only no-op, empty array safety, BinaryView parity, and non-view passthrough. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Skip gc() on non-sliced batches; add bloat-detection tests compact_string_view_columns now checks whether backing buffers are over-allocated before calling gc(). Non-sliced batches (common case) pay only an O(n) view scan instead of a full buffer copy. New tests: - view_needs_gc_detects_bloat: proves detection correctly identifies sliced arrays vs non-sliced arrays - non_sliced_batch_skips_gc: proves non-sliced batches pass through without allocation/copy Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Add stream_next integration test; fix non_sliced skip proof - New integration test (stringview_gc_test.rs): feeds a sliced 10K→100 StringView batch through df_stream_next and asserts the output backing buffers are compact (<10KB, not ~300KB). Fails immediately if compact_string_view_columns is removed from stream_next — the actual regression guard for this fix. - Fix non_sliced_batch_skips_gc: use Arc::ptr_eq to prove the fast path returns the original column without copying, not just that sizes match. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * retrigger CI Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Fix DiskUsageTests compilation: add missing NodeStats arg The makeNodeStatsWithResourceUsage helper was missing the AnalyticsBackendNativeMemoryStats parameter added in #21637. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> --------- Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

…project#21637) * Add native memory stat and task cancellation scaffolding Signed-off-by: Ajay Raj Nelapudi <ajnelapu@amazon.com> Co-authored-by: Bukhtawar Khan <bukhtawa@amazon.com>

…ject#21753) * Fix StringViewArray buffer bloat: gc() before FFI export StringViewArray::slice() shares ALL backing buffers via Arc::clone. When DataFusion's hash aggregate emits output (EmitTo::All + slice into 8192-row batches), each slice carries the full backing buffer pool. For a 14M-group aggregate with SearchPhrase strings, this means 174MB per batch instead of 0.4MB — a 435x amplification. Add compact_string_view_columns() in stream_next() that calls gc() on Utf8View/BinaryView columns before C Data Interface export. This compacts each batch to contain only its own referenced strings. Validated on 4-shard ClickBench q19: Before: 9748 batches × 174MB = 1.7TB, 12 minutes After: 9748 batches × 0.4MB = 4GB, 11 seconds (65x faster) Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Add Rust unit tests for compact_string_view_columns Regression guard for e2fd9bc (StringView buffer bloat fix). Tests prove that sliced StringView/BinaryView batches carry inflated backing buffers and that gc() compacts them to proportional size. Covers: large buffer compaction, inline-only no-op, empty array safety, BinaryView parity, and non-view passthrough. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Skip gc() on non-sliced batches; add bloat-detection tests compact_string_view_columns now checks whether backing buffers are over-allocated before calling gc(). Non-sliced batches (common case) pay only an O(n) view scan instead of a full buffer copy. New tests: - view_needs_gc_detects_bloat: proves detection correctly identifies sliced arrays vs non-sliced arrays - non_sliced_batch_skips_gc: proves non-sliced batches pass through without allocation/copy Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Add stream_next integration test; fix non_sliced skip proof - New integration test (stringview_gc_test.rs): feeds a sliced 10K→100 StringView batch through df_stream_next and asserts the output backing buffers are compact (<10KB, not ~300KB). Fails immediately if compact_string_view_columns is removed from stream_next — the actual regression guard for this fix. - Fix non_sliced_batch_skips_gc: use Arc::ptr_eq to prove the fast path returns the original column without copying, not just that sizes match. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * retrigger CI Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> * Fix DiskUsageTests compilation: add missing NodeStats arg The makeNodeStatsWithResourceUsage helper was missing the AnalyticsBackendNativeMemoryStats parameter added in opensearch-project#21637. Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com> --------- Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

AjayRajNelapudi requested a review from a team as a code owner May 13, 2026 11:03

himshikhagupta reviewed May 13, 2026

View reviewed changes

Comment thread ...ataformat-native/src/test/java/org/opensearch/nativebridge/spi/NativeMemoryServiceTests.java Outdated

Comment thread server/src/main/java/org/opensearch/plugin/stats/DataFusionNativeNodeStats.java Outdated

AjayRajNelapudi closed this May 14, 2026

AjayRajNelapudi reopened this May 14, 2026

AjayRajNelapudi force-pushed the stats/sample-metrics branch 6 times, most recently from dd1c96a to 5bc0743 Compare May 15, 2026 02:47

AjayRajNelapudi closed this May 15, 2026

AjayRajNelapudi reopened this May 15, 2026

AjayRajNelapudi closed this May 15, 2026

AjayRajNelapudi reopened this May 15, 2026

AjayRajNelapudi force-pushed the stats/sample-metrics branch from 5bc0743 to 963e148 Compare May 15, 2026 09:48

bharath-techie added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label May 15, 2026

AjayRajNelapudi closed this May 16, 2026

AjayRajNelapudi reopened this May 16, 2026

Bukhtawar mentioned this pull request May 16, 2026

Simplify task cancellation stats SPI: remove BackendStatsProvider indirection AjayRajNelapudi/OpenSearch#1

Merged

3 tasks

AjayRajNelapudi force-pushed the stats/sample-metrics branch from cf117b6 to 20c1442 Compare May 17, 2026 05:27

Mute flaky tests

298b746

Signed-off-by: Ajay Raj Nelapudi <ajnelapu@amazon.com>

AjayRajNelapudi force-pushed the stats/sample-metrics branch from 920793a to 298b746 Compare May 20, 2026 14:12

Refactor native_memory stats

4e4afe2

Signed-off-by: Ajay Raj Nelapudi <ajnelapu@amazon.com>

AjayRajNelapudi closed this May 20, 2026

AjayRajNelapudi reopened this May 20, 2026

Bukhtawar reviewed May 20, 2026

View reviewed changes

Comment thread server/src/main/java/org/opensearch/plugin/stats/AnalyticsBackendTaskCancellationStats.java

Bukhtawar approved these changes May 20, 2026

View reviewed changes

Bukhtawar merged commit 139bc58 into opensearch-project:main May 20, 2026
27 of 40 checks passed

cwperks mentioned this pull request May 21, 2026

Fix DiskUsageTests NodeStats constructor call #21770

Merged

3 tasks

opensearch-ci-bot mentioned this pull request Jun 29, 2026

[AUTOCUT] Gradle Check Flaky Test Report for AzureBlobStoreRepositoryTests #21372

Open

Uh oh!

Conversation

AjayRajNelapudi commented May 13, 2026

Description

Related Issues

Check List

Uh oh!

himshikhagupta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 4e4afe2)

Uh oh!

github-actions Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 16, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 17, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented May 14, 2026 •

edited

Loading

github-actions Bot commented May 16, 2026 •

edited

Loading

(Review updated until commit `4e4afe2`)

github-actions Bot commented May 16, 2026 •

edited

Loading