Skip to content

Fix NodeResourceUsageStats constructor to include nativeMemoryUtilizationPercent param - #21714

Merged
Bukhtawar merged 1 commit into
opensearch-project:mainfrom
GeekGlider:feature/tiered-storage-skeleton
May 18, 2026
Merged

Fix NodeResourceUsageStats constructor to include nativeMemoryUtilizationPercent param#21714
Bukhtawar merged 1 commit into
opensearch-project:mainfrom
GeekGlider:feature/tiered-storage-skeleton

Conversation

@GeekGlider

Copy link
Copy Markdown
Contributor

Description

Fixes compilation error in tiered storage integration tests caused by PR #21191 which added a 6th parameter (nativeMemoryUtilizationPercent) to the NodeResourceUsageStats constructor. Updated the constructor calls in HotToWarmTieringServiceIT and WarmToHotTieringServiceIT to pass 0.0 for the new parameter.

Related Issues

Fixes compilation failure introduced by #21191

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.

@GeekGlider
GeekGlider requested a review from a team as a code owner May 18, 2026 08:09
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit da78ece)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to da78ece
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use named constant for memory utilization

The hardcoded 0.0 for nativeMemoryUtilizationPercent may not accurately represent
test scenarios. Consider using a meaningful constant or variable that reflects the
intended test behavior, especially since this test validates JVM utilization breach
scenarios.

server/src/internalClusterTest/java/org/opensearch/storage/tiering/HotToWarmTieringServiceIT.java [1048]

-nodeId -> new NodeResourceUsageStats(nodeId, System.currentTimeMillis(), 99, 20, null, 0.0)
+private static final double DEFAULT_NATIVE_MEMORY_UTILIZATION = 0.0;
+...
+nodeId -> new NodeResourceUsageStats(nodeId, System.currentTimeMillis(), 99, 20, null, DEFAULT_NATIVE_MEMORY_UTILIZATION)
Suggestion importance[1-10]: 4

__

Why: While using a named constant could improve code readability, the suggestion has limited impact since 0.0 is a clear default value for native memory utilization in a test that focuses on JVM utilization breach. The improvement is marginal and mainly stylistic.

Low

Previous suggestions

Suggestions up to commit 34abae7
CategorySuggestion                                                                                                                                    Impact
General
Use named constant for memory utilization

The hardcoded 0.0 for nativeMemoryUtilizationPercent may not accurately represent
test scenarios. Consider using a meaningful constant or variable that reflects the
intended test behavior, especially since this test validates JVM utilization breach
scenarios.

server/src/internalClusterTest/java/org/opensearch/storage/tiering/HotToWarmTieringServiceIT.java [1048]

-nodeId -> new NodeResourceUsageStats(nodeId, System.currentTimeMillis(), 99, 20, null, 0.0)
+private static final double DEFAULT_NATIVE_MEMORY_UTILIZATION = 0.0;
+...
+nodeId -> new NodeResourceUsageStats(nodeId, System.currentTimeMillis(), 99, 20, null, DEFAULT_NATIVE_MEMORY_UTILIZATION)
Suggestion importance[1-10]: 4

__

Why: While using a named constant could improve code readability, the suggestion has limited impact since this is a test file where hardcoded values are common and acceptable. The 0.0 value is clear in context and doesn't significantly hinder understanding. The test focuses on JVM utilization (99%), not native memory utilization.

Low

@github-actions

Copy link
Copy Markdown
Contributor

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

…tionPercent param

Signed-off-by: Kavya Aggarwal <kavyaagg@amazon.com>
@GeekGlider
GeekGlider force-pushed the feature/tiered-storage-skeleton branch from 34abae7 to da78ece Compare May 18, 2026 09:13
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit da78ece

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for da78ece: SUCCESS

@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.46%. Comparing base (75b6e82) to head (da78ece).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21714      +/-   ##
============================================
- Coverage     73.56%   73.46%   -0.10%     
- Complexity    74882    74885       +3     
============================================
  Files          5994     5997       +3     
  Lines        339592   339688      +96     
  Branches      48948    48961      +13     
============================================
- Hits         249811   249547     -264     
- Misses        69893    70307     +414     
+ Partials      19888    19834      -54     

☔ 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.

@Bukhtawar
Bukhtawar merged commit 1364d7a into opensearch-project:main May 18, 2026
16 checks passed
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
…tionPercent param (opensearch-project#21714)

Signed-off-by: Kavya Aggarwal <kavyaagg@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