Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Depreceate NodeStatsFixedShardsMetricsCollector in favor of NodeStatsAllShardsMetricsCollector #551

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Depreceate NodeStatsFixedShardsMetricsCollector in favor of NodeStatsAllShardsMetricsCollector #551

merged 1 commit into from
Oct 2, 2023

Conversation

khushbr
Copy link
Collaborator

@khushbr khushbr commented Sep 30, 2023

NodeStatsAllShardsMetricsCollector is already released, redundant to run NodeStatsFixedShardsMetricsCollector. The Collector can further be removed in the future revision.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

@khushbr
Depreceate NodeStatsFixedShardsMetricsCollector in favor of NodeStats…
c5ecb14 
…AllShardsMetricsCollector

Signed-off-by: Khushboo Rajput <khushbr@amazon.com>
@khushbr khushbr self-assigned this Sep 30, 2023
@codecov
Copy link

codecov bot commented Sep 30, 2023 •
edited
Loading

Codecov Report

Merging #551 (06dffd0) into main (27a6b56) will decrease coverage by 4.88%.
The diff coverage is n/a.

❗ Current head 06dffd0 differs from pull request most recent head c5ecb14. Consider uploading reports for the commit c5ecb14 to get more accurate results

@@             Coverage Diff              @@
##               main     #551      +/-   ##
============================================
- Coverage     73.90%   69.02%   -4.88%     
+ Complexity      381      370      -11     
============================================
  Files            45       45              
  Lines          2698     2696       -2     
  Branches        172      172              
============================================
- Hits           1994     1861     -133     
- Misses          596      731     +135     
+ Partials        108      104       -4
Files Coverage Δ
...performanceanalyzer/PerformanceAnalyzerPlugin.java 76.92% <ø> (-0.22%) ⬇️
...collectors/NodeStatsAllShardsMetricsCollector.java 91.15% <ø> (ø)
...llectors/NodeStatsFixedShardsMetricsCollector.java 0.00% <ø> (-92.71%) ⬇️
...org/opensearch/performanceanalyzer/util/Utils.java 92.30% <ø> (-0.20%) ⬇️

... and 1 file with indirect coverage changes

arjunkumargiri
arjunkumargiri reviewed Oct 1, 2023
View reviewed changes
build.gradle
@@ -251,7 +251,7 @@ jacocoTestCoverageVerification {
violationRules {
rule {
limit {
minimum = 0.6
minimum = 0.5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better not to reduce coverage percentage

Copy link
Collaborator Author

@khushbr khushbr Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but in this case, the build on the repo depends on coverage verification and will fail now that we are ignoring the redundant NodeStatsFixedShardsMetricsCollectorTests.java.

The coverage increase will have to be separately addressed from this PR.

...java/org/opensearch/performanceanalyzer/collectors/NodeStatsFixedShardsMetricsCollector.java
@@ -34,7 +34,10 @@
import org.opensearch.performanceanalyzer.util.Utils;

/**
* This collector collects metrics for fixed number of shards on a node in a single run. These
* Note: 'NodeStatsAllShardsMetricsCollector' is already released and out of shadow mode, this class
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we cannot remove this as part of current release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the standard practice of marking the class deprecated in (n-1) release and finally removing in nth release.

@khushbr khushbr merged commit e7713ba into opensearch-project:main Oct 2, 2023
7 of 9 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 5, 2023
@khushbr @github-actions
Depreceate NodeStatsFixedShardsMetricsCollector in favor of NodeStats…
dd342b8 
…AllShardsMetricsCollector (#551)

Signed-off-by: Khushboo Rajput <khushbr@amazon.com>
(cherry picked from commit e7713ba)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 5, 2023
Merged
khushbr added a commit that referenced this pull request Oct 5, 2023
@opensearch-trigger-bot @khushbr
Depreceate NodeStatsFixedShardsMetricsCollector in favor of NodeStats…
5d80ec5 
…AllShardsMetricsCollector (#551) (#559)

Signed-off-by: Khushboo Rajput <khushbr@amazon.com>
(cherry picked from commit e7713ba)

Co-authored-by: Khushboo Rajput <59671881+khushbr@users.noreply.github.com>
@khushbr khushbr added the v2.11.0 Issues targeting release v2.11.0 label Oct 5, 2023
@chibacchie chibacchie mentioned this pull request Jan 28, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbcd90 sbcd90 sbcd90 approved these changes

@ansjcy ansjcy ansjcy approved these changes

@arjunkumargiri arjunkumargiri arjunkumargiri approved these changes

@praveensameneni praveensameneni Awaiting requested review from praveensameneni

@goyamegh goyamegh Awaiting requested review from goyamegh

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02

@sgup432 sgup432 Awaiting requested review from sgup432

Assignees

@khushbr khushbr

Labels
backport 1.x backport 2.x v2.11.0 Issues targeting release v2.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@khushbr @sbcd90 @ansjcy @arjunkumargiri