Skip to content

Add composite-engine stats and parquet ingest rejection/pool metrics - #22134

Merged
mgodwan merged 1 commit into
opensearch-project:mainfrom
ask-kamal-nayan:native-stats
Jun 19, 2026
Merged

Add composite-engine stats and parquet ingest rejection/pool metrics#22134
mgodwan merged 1 commit into
opensearch-project:mainfrom
ask-kamal-nayan:native-stats

Conversation

@ask-kamal-nayan

Copy link
Copy Markdown
Contributor

Description

1. Composite-engine stats provider (new)

Exposes per-format stats for the composite engine via GET /_plugins/composite/{index}/_stats and GET /_plugins/composite/_nodes/_stats.

Block Counters
refresh refresh_total, refresh_time_millis
refresh (merge-on-refresh breakdown) refresh_merge_total, refresh_merge_time_millis
merge merge_total, merge_time_millis, merge_failures
write write_primary_failures, write_secondary_failures
mapping mapping_update_executed_total (counts only actually-applied dynamic mapping updates)

2. Parquet native_write_rejections + bounded write pool

  • Adds native_write.native_write_rejections, incremented when the parquet_native_write thread pool rejects a background write.
  • Bounds that pool's queue at 10k (previously unbounded). Under sustained saturation, ingestion now sheds load with HTTP 429 backpressure instead of queueing unboundedly.

⚠️ Behavior change: ingestion now rejects under overload rather than queueing indefinitely.

3. Parquet node-level ingest pool stats

Adds a native_ingest_pool block to parquet per-node stats reporting the live parquet_native_write pool: threads, queue_depth, active, rejected, largest_queue, completed — giving visibility into ingest backpressure before rejections occur.

Notes

Timing is recorded via the shared StatsRecorder utility. There are no changes to the indexing/refresh/merge/flush execution flow beyond the intentional pool bound — all instrumentation is side-effect-free counter emission.

Testing

  • precommit clean on both plugins
  • 328 unit tests (incl. new CompositeShardStatsTests + ParquetIngestPoolStatsTests)
  • 59 stats integration tests (incl. new CompositeStatsEndpointIT)

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.

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 4c52a4c)

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 Resource Leak

If an exception is thrown after registering the stats tracker but before the constructor completes (e.g., during merger initialization), the rollback logic attempts to unregister. However, if provider.unregister(shardId) itself throws, the original exception is suppressed and the rollback exception is only logged. This leaves the tracker registered in the provider's map, causing a resource leak. The original exception should be preserved and re-thrown after logging the rollback failure.

// Register the per-shard tracker so REST endpoints can read live counters; unregistered
// in close(). Rolls back the registration if anything below throws, to avoid leaking it.
CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
boolean registered = false;
try {
    if (provider != null && shardId != null) {
        provider.register(shardId, statsTracker);
        registered = true;
    }
} catch (Throwable t) {
    if (registered) {
        try {
            provider.unregister(shardId);
        } catch (Throwable rollbackErr) {
            logger.warn("Failed to unregister composite stats tracker during constructor rollback", rollbackErr);
        }
    }
    throw t;
}
Inconsistent Rejection Counting

When the thread pool rejects a write task, stats.incNativeWriteRejections() is called before re-throwing the exception. However, the rejection is counted even though the write never entered the pool's queue or executed. If the caller retries the operation after receiving the rejection, the same logical write attempt could be counted multiple times. This inflates the rejection metric beyond the actual number of distinct rejected writes.

try {
    pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
} catch (OpenSearchRejectedExecutionException e) {
    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
    stats.incNativeWriteRejections();
    throw e;
}

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 4c52a4c

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Clean up frozen VSR on rejection

When the thread pool rejects the write task, the frozenVSR remains frozen and
pendingWrite stays null, but no cleanup is performed. This leaves the VSR pool in an
inconsistent state. Add cleanup logic to unfreeze the VSR and handle the frozen
state properly before re-throwing.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [324-330]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
+    // Pool saturated — count the rejection, clean up frozen VSR, and re-throw.
     stats.incNativeWriteRejections();
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 8

__

Why: This identifies a real resource leak. When submit() throws OpenSearchRejectedExecutionException, the frozenVSR remains frozen in the pool without cleanup, leaving the VSR pool in an inconsistent state. Adding vsrPool.unsetFrozenVSR() before re-throwing ensures proper cleanup and prevents the pool from getting stuck.

Medium
Fix registration rollback race condition

The rollback logic has a race condition. If provider.register() succeeds but an
exception occurs before registered = true is set, the tracker won't be unregistered.
Move the registered = true assignment immediately after the successful registration
call to ensure proper cleanup.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [165-180]

 CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
 boolean registered = false;
 try {
     if (provider != null && shardId != null) {
         provider.register(shardId, statsTracker);
         registered = true;
     }
 } catch (Throwable t) {
-    if (registered) {
+    if (registered && provider != null) {
         try {
             provider.unregister(shardId);
         } catch (Throwable rollbackErr) {
             logger.warn("Failed to unregister composite stats tracker during constructor rollback", rollbackErr);
         }
     }
     throw t;
 }
Suggestion importance[1-10]: 3

__

Why: The concern about a race condition is overstated. The registered = true assignment occurs immediately after provider.register() in the same thread with no intervening operations that could throw. The added null-check for provider in the rollback is a minor improvement but doesn't address a real race condition. The impact is minimal.

Low
General
Prevent resource leak on unregister failure

If provider.unregister() throws an exception, the primary and secondary engines
won't be closed, causing resource leaks. Wrap the unregister call in a try-catch or
use IOUtils.closeWhileHandlingException to ensure engines are always closed.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [521-527]

-CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-if (provider != null && shardId != null) {
-    provider.unregister(shardId);
+try {
+    CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
+    if (provider != null && shardId != null) {
+        provider.unregister(shardId);
+    }
+} catch (Exception e) {
+    logger.warn("Failed to unregister composite stats tracker during close", e);
 }
 IOUtils.closeWhileHandlingException(primaryEngine);
Suggestion importance[1-10]: 6

__

Why: If provider.unregister() throws an exception, the engines won't be closed, causing a resource leak. Wrapping the unregister call in try-catch ensures engines are always closed. This is a valid concern, though the likelihood of unregister() throwing is low since it's a simple map removal.

Low

Previous suggestions

Suggestions up to commit 4c52a4c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Clean up frozen VSR on rejection

When the thread pool rejects the write task, the frozenVSR remains frozen and is
never completed or unfrozen. This leaves the VSR pool in an inconsistent state. Add
cleanup logic in the catch block to call vsrPool.completeVSR(frozenVSR) and
vsrPool.unsetFrozenVSR() before re-throwing the exception.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [324-330]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
+    // Pool saturated — count the rejection, clean up frozen VSR, and re-throw.
     stats.incNativeWriteRejections();
+    vsrPool.completeVSR(frozenVSR);
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 9

__

Why: When the thread pool rejects the write task, frozenVSR remains frozen and is never completed or unfrozen, leaving the VSR pool in an inconsistent state. This is a critical resource leak that could prevent future VSR rotations. The fix properly cleans up the frozen VSR before re-throwing.

High
Fix rollback logic in constructor

The rollback logic has a flaw: registered is set to true only after
provider.register() succeeds, but the catch block checks registered before
attempting unregister. If provider.register() throws, registered remains false and
the rollback is skipped. Move the registered = true assignment before the register
call, or remove the flag and always attempt unregister in the catch block.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [164-180]

 CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-boolean registered = false;
 try {
     if (provider != null && shardId != null) {
         provider.register(shardId, statsTracker);
-        registered = true;
     }
 } catch (Throwable t) {
-    if (registered) {
+    if (provider != null && shardId != null) {
         try {
             provider.unregister(shardId);
         } catch (Throwable rollbackErr) {
             logger.warn("Failed to unregister composite stats tracker during constructor rollback", rollbackErr);
         }
     }
     throw t;
 }
Suggestion importance[1-10]: 8

__

Why: The rollback logic is flawed: registered is set to true only after provider.register() succeeds, so if registration throws, the catch block won't attempt unregister. This could leak the tracker registration. The fix correctly ensures cleanup always happens when needed.

Medium
Ensure thread-safe singleton initialization

The singleton initialization is not thread-safe. Multiple threads could
simultaneously see INSTANCE == null and both proceed to set it and register with the
registry, causing duplicate registrations. Use double-checked locking with
synchronized block or a static initializer to ensure thread-safe singleton creation.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/stats/CompositeStatsProvider.java [43-49]

 public CompositeStatsProvider() {
-    // First instance wins. Subsequent constructions are no-ops on the singleton slot.
-    if (INSTANCE == null) {
-        INSTANCE = this;
+    synchronized (CompositeStatsProvider.class) {
+        if (INSTANCE == null) {
+            INSTANCE = this;
+            DataFormatStatsProviderRegistry.INSTANCE.register(this);
+        }
     }
-    DataFormatStatsProviderRegistry.INSTANCE.register(this);
 }
Suggestion importance[1-10]: 7

__

Why: The singleton pattern lacks thread-safety. Multiple threads could simultaneously see INSTANCE == null and create duplicate instances, causing multiple registry registrations. The suggested synchronized block ensures only one instance is created and registered.

Medium
Suggestions up to commit 8c07c42
CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent resource leak on unregister failure

If provider.unregister() throws an exception, the primary engine, secondary engines,
and committer will never be closed, causing resource leaks. The unregister call
should be wrapped in a try-catch or moved after the resource cleanup to ensure
engines are always closed.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [520-528]

 public void close() throws IOException {
-    CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-    if (provider != null && shardId != null) {
-        provider.unregister(shardId);
+    try {
+        CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
+        if (provider != null && shardId != null) {
+            provider.unregister(shardId);
+        }
+    } catch (Exception e) {
+        logger.warn("Failed to unregister composite stats tracker during close", e);
     }
     IOUtils.closeWhileHandlingException(primaryEngine);
     secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
     IOUtils.closeWhileHandlingException(committer);
 }
Suggestion importance[1-10]: 9

__

Why: This is a critical resource leak issue. If provider.unregister() throws an exception, the primaryEngine, secondaryEngines, and committer will never be closed, leaking resources. The suggestion to wrap the unregister call in a try-catch ensures that engine cleanup always occurs, preventing resource leaks. This is a high-impact correctness fix.

High
Restore VSR pool state on rejection

When the thread pool rejects the write task, the frozenVSR remains frozen and is
never completed or unfrozen. This leaves the VSR pool in an inconsistent state. The
rejection handler should call vsrPool.completeVSR(frozenVSR) and
vsrPool.unsetFrozenVSR() before re-throwing to restore the pool state.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [307-313]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
     stats.incNativeWriteRejections();
+    vsrPool.completeVSR(frozenVSR);
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 9

__

Why: When the thread pool rejects the write task, the frozenVSR remains in a frozen state and is never completed or unfrozen, leaving the VSR pool in an inconsistent state. The suggestion to call vsrPool.completeVSR(frozenVSR) and vsrPool.unsetFrozenVSR() before re-throwing the exception is correct and critical for maintaining pool consistency. This is a high-impact correctness fix.

High
Remove flawed rollback logic

The rollback logic has a critical flaw: registered is set to true only after
provider.register() succeeds, but the catch block checks registered to decide
whether to unregister. If provider.register() throws, registered remains false, so
the unregister rollback never runs. However, the real issue is that if anything
after the registration throws (e.g., later constructor code), the tracker remains
registered but the engine is never fully constructed, causing a resource leak.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [163-180]

 CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-boolean registered = false;
-try {
-    if (provider != null && shardId != null) {
-        provider.register(shardId, statsTracker);
-        registered = true;
-    }
-} catch (Throwable t) {
-    if (registered) {
-        try {
-            provider.unregister(shardId);
-        } catch (Throwable rollbackErr) {
-            logger.warn("Failed to unregister composite stats tracker during constructor rollback", rollbackErr);
-        }
-    }
-    throw t;
+if (provider != null && shardId != null) {
+    provider.register(shardId, statsTracker);
 }
Suggestion importance[1-10]: 8

__

Why: The rollback logic is fundamentally flawed. If provider.register() throws, registered remains false and no unregister occurs. More critically, if any code after registration throws (not shown in the snippet but implied by the try-catch scope), the tracker remains registered while the engine is never fully constructed, causing a resource leak. The suggestion to remove the rollback is correct because the registration should either succeed or the constructor should fail cleanly without partial state.

Medium
Suggestions up to commit e456288
CategorySuggestion                                                                                                                                    Impact
Possible issue
Resource leak if unregister throws

If provider.unregister() throws an exception, the engines and committer will not be
closed, causing resource leaks. Wrap the unregister call in a try-catch block or
move it after the resource cleanup to ensure engines are always closed.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [519-527]

 public void close() throws IOException {
-    CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-    if (provider != null && shardId != null) {
-        provider.unregister(shardId);
+    try {
+        IOUtils.closeWhileHandlingException(primaryEngine);
+        secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
+        IOUtils.closeWhileHandlingException(committer);
+    } finally {
+        CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
+        if (provider != null && shardId != null) {
+            try {
+                provider.unregister(shardId);
+            } catch (Exception e) {
+                logger.warn("Failed to unregister stats tracker during close", e);
+            }
+        }
     }
-    IOUtils.closeWhileHandlingException(primaryEngine);
-    secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
-    IOUtils.closeWhileHandlingException(committer);
 }
Suggestion importance[1-10]: 9

__

Why: If provider.unregister() throws an exception, the primaryEngine, secondaryEngines, and committer will not be closed, causing a critical resource leak. The suggestion to move unregister to a finally block ensures resources are always cleaned up, which is essential for system stability.

High
Frozen VSR not released on rejection

After catching OpenSearchRejectedExecutionException and incrementing the rejection
counter, the frozen VSR remains locked and is never completed or released. This
causes a resource leak. Ensure vsrPool.completeVSR(frozenVSR) and
vsrPool.unsetFrozenVSR() are called in a finally block or on the rejection path.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [307-313]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
     stats.incNativeWriteRejections();
+    vsrPool.completeVSR(frozenVSR);
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 9

__

Why: When OpenSearchRejectedExecutionException is caught, the frozen VSR is never completed or released, causing a resource leak. The suggestion to call vsrPool.completeVSR(frozenVSR) and vsrPool.unsetFrozenVSR() on the rejection path is critical to prevent resource exhaustion.

High
Unreachable rollback logic in constructor

The rollback logic is unreachable because registered is only set to true after
provider.register() succeeds. If register() throws, registered remains false, so the
rollback block never executes. Move the registered = true assignment before the
register() call, or remove the rollback block entirely if registration is atomic.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [164-180]

 CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-boolean registered = false;
 try {
     if (provider != null && shardId != null) {
         provider.register(shardId, statsTracker);
-        registered = true;
     }
 } catch (Throwable t) {
-    if (registered) {
-        try {
-            provider.unregister(shardId);
-        } catch (Throwable rollbackErr) {
-            logger.warn("Failed to unregister composite stats tracker during constructor rollback", rollbackErr);
-        }
-    }
     throw t;
 }
Suggestion importance[1-10]: 8

__

Why: The rollback logic is indeed unreachable because registered is set to true only after provider.register() succeeds. If register() throws, the rollback block never executes, making the code misleading and potentially masking issues. This is a significant logic flaw that should be fixed.

Medium
Suggestions up to commit 456a043
CategorySuggestion                                                                                                                                    Impact
Possible issue
Clean up frozen VSR on rejection

The rejection counter is incremented but the frozen VSR state is not cleaned up. If
the pool rejects the write task, vsrPool.unsetFrozenVSR() is never called (it's
inside writeTask), leaving the pool in an inconsistent state. Consider calling
vsrPool.unsetFrozenVSR() in the catch block before re-throwing.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [307-313]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // Pool saturated — count the rejection and re-throw (surfaces as HTTP 429).
+    // Pool saturated — count the rejection, clean up frozen state, and re-throw.
     stats.incNativeWriteRejections();
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 9

__

Why: Critical resource leak: when the pool rejects the write task, vsrPool.unsetFrozenVSR() is never called (it's inside writeTask), leaving the pool in an inconsistent state. This can cause subsequent operations to fail or hang.

High
General
Ensure resources close on unregister failure

If provider.unregister() throws an exception, the engine resources
(primary/secondary engines, committer) are never closed. Move the unregister call
inside a try-finally block or use IOUtils.closeWhileHandlingException to ensure
cleanup always happens.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [507-514]

 public void close() throws IOException {
-    CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-    if (provider != null && shardId != null) {
-        provider.unregister(shardId);
+    try {
+        CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
+        if (provider != null && shardId != null) {
+            provider.unregister(shardId);
+        }
+    } finally {
+        IOUtils.closeWhileHandlingException(primaryEngine);
+        secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
+        IOUtils.closeWhileHandlingException(committer);
     }
-    IOUtils.closeWhileHandlingException(primaryEngine);
-    secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
-    IOUtils.closeWhileHandlingException(committer);
 }
Suggestion importance[1-10]: 8

__

Why: If provider.unregister() throws, the engine resources (primaryEngine, secondaryEngines, committer) are never closed, causing resource leaks. Using try-finally ensures cleanup always happens.

Medium
Handle registration failure in constructor

If provider.register() throws an exception, the engine constructor completes but the
tracker is not registered. This leaves the engine in a partially initialized state.
Consider wrapping registration in a try-catch and closing already-constructed
resources on failure.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [164-167]

 CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
 if (provider != null && shardId != null) {
-    provider.register(shardId, statsTracker);
+    try {
+        provider.register(shardId, statsTracker);
+    } catch (Exception e) {
+        IOUtils.closeWhileHandlingException(primaryEngine);
+        secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
+        IOUtils.closeWhileHandlingException(committer);
+        throw e;
+    }
 }
Suggestion importance[1-10]: 7

__

Why: If provider.register() throws, the engine is left partially initialized with unclosed resources. While unlikely, wrapping in try-catch and cleaning up on failure improves robustness.

Medium
Suggestions up to commit 2966865
CategorySuggestion                                                                                                                                    Impact
Possible issue
Clean up frozen VSR on rejection

When the thread pool rejects the write task, the frozen VSR remains in the pool
without being completed or unfrozen. This leaves the VSR pool in an inconsistent
state. Clean up the frozen VSR state before re-throwing the exception to prevent
resource leaks.

sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java [307-314]

 try {
     pendingWrite = threadPool.executor(vsrRotationThread).submit(writeTask);
 } catch (OpenSearchRejectedExecutionException e) {
-    // parquet_native_write pool saturated (all workers busy + queue full). Count and
-    // re-throw so the caller still sees the rejection (HTTP 429) as ingest backpressure.
     stats.incNativeWriteRejections();
+    vsrPool.unsetFrozenVSR();
     throw e;
 }
Suggestion importance[1-10]: 8

__

Why: This identifies a real resource leak issue. When submit() throws OpenSearchRejectedExecutionException, the frozenVSR remains set in the pool without being completed or unset, leaving the VSR pool in an inconsistent state. The fix correctly calls vsrPool.unsetFrozenVSR() to clean up before re-throwing.

Medium
Fix stats unregistration race condition

The stats tracker is unregistered before closing the engines, which could lead to a
race condition where stats are still being recorded during engine shutdown. Move the
unregister call after all engines and the committer are closed to ensure no stats
updates occur after unregistration.

sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeIndexingExecutionEngine.java [511-519]

 public void close() throws IOException {
-    CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
-    if (provider != null && shardId != null) {
-        provider.unregister(shardId);
+    try {
+        IOUtils.closeWhileHandlingException(primaryEngine);
+        secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
+        IOUtils.closeWhileHandlingException(committer);
+    } finally {
+        CompositeStatsProvider provider = CompositeStatsProvider.getInstance();
+        if (provider != null && shardId != null) {
+            provider.unregister(shardId);
+        }
     }
-    IOUtils.closeWhileHandlingException(primaryEngine);
-    secondaryEngines.forEach(IOUtils::closeWhileHandlingException);
-    IOUtils.closeWhileHandlingException(committer);
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential race condition where stats could be recorded during engine shutdown. Using a finally block ensures cleanup happens even if closing throws, improving robustness. However, the impact is moderate since closeWhileHandlingException already handles exceptions gracefully.

Medium

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 2966865: SUCCESS

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (7eb89bb) to head (4c52a4c).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22134      +/-   ##
============================================
- Coverage     73.43%   73.31%   -0.12%     
+ Complexity    75926    75827      -99     
============================================
  Files          6070     6070              
  Lines        344613   344613              
  Branches      49579    49579              
============================================
- Hits         253057   252653     -404     
- Misses        71400    71807     +407     
+ Partials      20156    20153       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 456a043

@github-actions

Copy link
Copy Markdown
Contributor

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

@ask-kamal-nayan
ask-kamal-nayan force-pushed the native-stats branch 2 times, most recently from c3ae008 to e456288 Compare June 15, 2026 06:44
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e456288

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for e456288: null

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

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8c07c42

Signed-off-by: Kamal Nayan <askkamal@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4c52a4c

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 4c52a4c: null

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

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4c52a4c

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 4c52a4c: SUCCESS

@mgodwan
mgodwan marked this pull request as ready for review June 19, 2026 17:06
@mgodwan
mgodwan requested a review from a team as a code owner June 19, 2026 17:06
@mgodwan
mgodwan merged commit ae22a78 into opensearch-project:main Jun 19, 2026
29 of 31 checks passed
OVyshnevskyi pushed a commit to OVyshnevskyi/OpenSearch that referenced this pull request Jun 22, 2026
…pensearch-project#22134)

Signed-off-by: Kamal Nayan <askkamal@amazon.com>
Co-authored-by: Kamal Nayan <askkamal@amazon.com>
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
…pensearch-project#22134)

Signed-off-by: Kamal Nayan <askkamal@amazon.com>
Co-authored-by: Kamal Nayan <askkamal@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants