Add current_application_duration_ms to cluster state download stats in node stats API

[Ayushiarya246]

Description

This PR adds a new metric current_application_duration_ms to the cluster state download stats exposed via the Node Stats API (_nodes/stats/discovery).

Related Issues

Resolves #20527

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]

PR Reviewer Guide 🔍

(Review updated until commit 1eab44a)

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 Race Condition Reading applicationStartTimeNanos in getCurrentApplicationDurationMs() is not synchronized with writes in runTask(). If a thread reads the value just as another thread writes NOT_RUNNING, the calculation System.nanoTime() - startNanos could use a stale startNanos from a previous run, yielding an incorrect duration. This manifests when getCurrentApplicationDurationMs() is called concurrently with task completion. @Override public long getCurrentApplicationDurationMs() { long startNanos = this.applicationStartTimeNanos; if (startNanos == NOT_RUNNING) { return 0; } return TimeValue.nsecToMSec(System.nanoTime() - startNanos); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1eab44a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Prevent negative duration from race condition Reading applicationStartTimeNanos without synchronization while it's being updated by runTask() can lead to visibility issues. Although marked volatile, the calculation System.nanoTime() - startNanos could return negative values if the reset to NOT_RUNNING happens between reading startNanos and calling System.nanoTime(). Add a check to ensure non-negative results. server/src/main/java/org/opensearch/cluster/service/ClusterApplierService.java [800-807] @Override public long getCurrentApplicationDurationMs() { long startNanos = this.applicationStartTimeNanos; if (startNanos == NOT_RUNNING) { return 0; } - return TimeValue.nsecToMSec(System.nanoTime() - startNanos); + long duration = TimeValue.nsecToMSec(System.nanoTime() - startNanos); + return Math.max(0, duration); } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a valid edge case where a race condition between reading startNanos and resetting to NOT_RUNNING could theoretically produce negative durations. Adding Math.max(0, duration) is a defensive programming practice that prevents returning negative values, though the likelihood of this race is low due to the volatile field.	Low
	Move timestamp capture closer to application The applicationStartTimeNanos is set before the actual cluster state application begins. This may include time spent in logging and state retrieval, leading to inflated duration measurements. Consider moving the timestamp capture to immediately before the actual application logic starts. server/src/main/java/org/opensearch/cluster/service/ClusterApplierService.java [475-477] -this.applicationStartTimeNanos = System.nanoTime(); logger.debug("processing [{}]: execute", task.source); final ClusterState previousClusterState = state.get(); +this.applicationStartTimeNanos = System.nanoTime(); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that applicationStartTimeNanos is set before logging and state retrieval, which could inflate duration measurements. However, the impact is minimal since logging is typically fast, and the measurement still captures the overall task processing time which may be intentional.	Low

---

Previous suggestions

Suggestions up to commit 238de71

Category	Suggestion	Impact
General	Prevent negative duration from race conditions The calculation System.nanoTime() - startNanos can produce negative values if the system clock wraps around or if there's a race condition where applicationStartTimeNanos is reset between reading it and calling System.nanoTime(). Add a check to ensure the result is non-negative before returning. server/src/main/java/org/opensearch/cluster/service/ClusterApplierService.java [800-806] @Override public long getCurrentApplicationDurationMs() { long startNanos = this.applicationStartTimeNanos; if (startNanos == NOT_RUNNING) { return 0; } - return TimeValue.nsecToMSec(System.nanoTime() - startNanos); + long durationMs = TimeValue.nsecToMSec(System.nanoTime() - startNanos); + return Math.max(0, durationMs); } Suggestion importance[1-10]: 5 __ Why: Adding Math.max(0, durationMs) provides defensive programming against potential negative values from race conditions or clock issues. While the likelihood is low, this is a reasonable safety measure for duration calculations.	Low

Suggestions up to commit 4492c13

Category	Suggestion	Impact
General	Prevent negative duration values The calculation System.nanoTime() - startNanos could theoretically overflow or produce negative values if System.nanoTime() wraps around, though this is extremely rare. Add a safeguard to ensure the returned duration is never negative. server/src/main/java/org/opensearch/cluster/service/ClusterApplierService.java [800-807] @Override public long getCurrentApplicationDurationMs() { long startNanos = this.applicationStartTimeNanos; if (startNanos == NOT_RUNNING) { return 0; } - return TimeValue.nsecToMSec(System.nanoTime() - startNanos); + long durationNanos = System.nanoTime() - startNanos; + return Math.max(0, TimeValue.nsecToMSec(durationNanos)); } Suggestion importance[1-10]: 5 __ Why: Adding Math.max(0, ...) is a defensive programming practice that prevents negative values from being returned, though System.nanoTime() wraparound is extremely rare. This is a minor improvement for robustness.	Low

Suggestions up to commit 81545b0

Category	Suggestion	Impact
General	Potential race condition in stats mutation The method mutates the shared stats object returned by getRemoteFullDownloadStats() which could cause race conditions if multiple threads call this method concurrently. Consider creating a defensive copy or using thread-safe updates to prevent concurrent modification issues. server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java [2120-2124] public PersistedStateStats getFullDownloadStats() { RemoteDownloadStats stats = (RemoteDownloadStats) remoteStateStats.getRemoteFullDownloadStats(); - stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); + long currentDuration = applicationDurationMsSupplier.getAsLong(); + stats.setCurrentApplicationDurationMs(currentDuration); return stats; } Suggestion importance[1-10]: 3 __ Why: While the concern about concurrent access is valid, the improved_code doesn't actually address the race condition - it just extracts the supplier call to a local variable, which doesn't prevent concurrent mutation of the shared stats object.	Low
	Handle potential nanoTime overflow The calculation System.nanoTime() - startNanos could theoretically overflow if the application runs for an extremely long time (years). While unlikely, consider adding a safeguard to handle potential negative results from the subtraction or document the assumption that applications complete within reasonable timeframes. server/src/main/java/org/opensearch/cluster/service/ClusterApplierService.java [800-807] @Override public long getCurrentApplicationDurationMs() { long startNanos = this.applicationStartTimeNanos; if (startNanos == NOT_RUNNING) { return 0; } - return TimeValue.nsecToMSec(System.nanoTime() - startNanos); + long durationNanos = System.nanoTime() - startNanos; + return durationNanos >= 0 ? TimeValue.nsecToMSec(durationNanos) : 0; } Suggestion importance[1-10]: 2 __ Why: The concern about System.nanoTime() overflow is extremely theoretical and unlikely in practice. The suggested safeguard adds unnecessary complexity for a scenario that would require years of continuous operation without resetting the timer.	Low

Suggestions up to commit a2ed811

Category	Suggestion	Impact
General	Avoid mutating shared stats objects Directly mutating the stats object returned from remoteStateStats can cause thread-safety issues and unexpected side effects if the same object is accessed concurrently. Consider creating a defensive copy or using immutable patterns to avoid modifying shared state. server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java [2120-2130] public PersistedStateStats getFullDownloadStats() { RemoteDownloadStats stats = (RemoteDownloadStats) remoteStateStats.getRemoteFullDownloadStats(); - stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); + long currentDuration = applicationDurationMsSupplier.getAsLong(); + stats.setCurrentApplicationDurationMs(currentDuration); return stats; } public PersistedStateStats getDiffDownloadStats() { RemoteDownloadStats stats = (RemoteDownloadStats) remoteStateStats.getRemoteDiffDownloadStats(); - stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); + long currentDuration = applicationDurationMsSupplier.getAsLong(); + stats.setCurrentApplicationDurationMs(currentDuration); return stats; } Suggestion importance[1-10]: 3 __ Why: While the suggestion raises a valid concern about thread-safety when mutating shared objects, the improved_code only extracts the supplier call to a local variable without addressing the actual mutation issue. The suggestion doesn't provide a meaningful solution like creating a defensive copy.	Low

Suggestions up to commit 1d95708

Category	Suggestion	Impact
General	Add type check before casting The methods directly cast and mutate the stats object returned by remoteStateStats, which could cause issues if the same instance is shared or cached. Consider creating a defensive copy or ensuring the stats object is not reused to prevent unintended side effects. server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java [2121-2130] public PersistedStateStats getFullDownloadStats() { - RemoteDownloadStats stats = (RemoteDownloadStats) remoteStateStats.getRemoteFullDownloadStats(); - stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); - return stats; + PersistedStateStats baseStats = remoteStateStats.getRemoteFullDownloadStats(); + if (baseStats instanceof RemoteDownloadStats) { + RemoteDownloadStats stats = (RemoteDownloadStats) baseStats; + stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); + } + return baseStats; } public PersistedStateStats getDiffDownloadStats() { - RemoteDownloadStats stats = (RemoteDownloadStats) remoteStateStats.getRemoteDiffDownloadStats(); - stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); - return stats; + PersistedStateStats baseStats = remoteStateStats.getRemoteDiffDownloadStats(); + if (baseStats instanceof RemoteDownloadStats) { + RemoteDownloadStats stats = (RemoteDownloadStats) baseStats; + stats.setCurrentApplicationDurationMs(applicationDurationMsSupplier.getAsLong()); + } + return baseStats; } Suggestion importance[1-10]: 5 __ Why: Adding an instanceof check before casting improves defensive programming and prevents potential ClassCastException. However, the impact is moderate since the code likely controls the type returned by remoteStateStats, making this primarily a safety enhancement rather than fixing a critical bug.	Low

[github-actions]

Persistent review updated to latest commit 18c1649

[github-actions]

❌ Gradle check result for 18c1649: 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]

Persistent review updated to latest commit 4868dfc

[github-actions]

❌ Gradle check result for 4868dfc: 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]

Persistent review updated to latest commit 4d82611

[github-actions]

❌ Gradle check result for 4d82611: 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]

Persistent review updated to latest commit dd9e6d6

[github-actions]

❌ Gradle check result for dd9e6d6: 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]

Persistent review updated to latest commit f081389

[github-actions]

❌ Gradle check result for f081389: 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]

Persistent review updated to latest commit 3c881d2

[github-actions]

❌ Gradle check result for 3c881d2: 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]

Persistent review updated to latest commit 0e968dc

[github-actions]

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

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.44%. Comparing base (ac2b2fd) to head (1eab44a).

Files with missing lines	Patch %	Lines
...org/opensearch/cluster/service/ClusterApplier.java	0.00%	1 Missing ⚠️
...opensearch/gateway/remote/RemoteDownloadStats.java	80.00%	1 Missing ⚠️
server/src/main/java/org/opensearch/node/Node.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20922      +/-   ##
============================================
+ Coverage     73.37%   73.44%   +0.06%     
- Complexity    74808    74821      +13     
============================================
  Files          6007     6007              
  Lines        339994   340016      +22     
  Branches      48987    48988       +1     
============================================
+ Hits         249469   249718     +249     
+ Misses        70672    70409     -263     
- Partials      19853    19889      +36     

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

🚀 New features to boost your workflow:

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

[github-actions]

Persistent review updated to latest commit bb04960

[github-actions]

✅ Gradle check result for bb04960: SUCCESS

[github-actions]

Persistent review updated to latest commit 1d95708

[github-actions]

❌ Gradle check result for 1d95708: 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]

Persistent review updated to latest commit ebfe687

[github-actions]

Persistent review updated to latest commit a2ed811

[github-actions]

❌ Gradle check result for a2ed811: 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]

Persistent review updated to latest commit 81545b0

[github-actions]

❌ Gradle check result for 81545b0: 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]

Persistent review updated to latest commit 4492c13

[github-actions]

Persistent review updated to latest commit 238de71

[github-actions]

✅ Gradle check result for 238de71: SUCCESS

[pradeep-L]

LGTM

[github-actions]

Persistent review updated to latest commit 1eab44a

[github-actions]

✅ Gradle check result for 1eab44a: SUCCESS