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

GEODE-9190 Cleanup of Stats Code Warnings #6347

Merged
merged 8 commits into from Apr 30, 2021
Merged

GEODE-9190 Cleanup of Stats Code Warnings #6347

merged 8 commits into from Apr 30, 2021

Conversation

mhansonp
Copy link
Contributor

I need to do other stat adding and I kept coming across these stats files with many warnings.

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2021

This pull request introduces 1 alert when merging 2835189 into 5f7abc9 - view on LGTM.com

new alerts:

  • 1 for Result of multiplication cast to wider type

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2021

This pull request introduces 1 alert when merging 463a5a1 into 5f7abc9 - view on LGTM.com

new alerts:

  • 1 for Result of multiplication cast to wider type

@mhansonp mhansonp removed the request for review from albertogpz April 19, 2021 22:40
@mhansonp mhansonp marked this pull request as ready for review April 20, 2021 05:03
DonalEvans
DonalEvans requested changes Apr 20, 2021
View reviewed changes
Copy link
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

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

A few typos and some unintentionally included changes. Also, could the title of the PR please be changed to reference the associated GEODE ticket and be more descriptive?

geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java Outdated Show resolved Hide resolved
@DonalEvans
Copy link
Contributor

I've approved the code changes, but please also update the name of the PR to include the GEODE ticket associated with these changes.

Bill
Bill reviewed Apr 21, 2021
View reviewed changes
Copy link
Contributor

@Bill Bill left a comment

Choose a reason for hiding this comment

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

This looks good @mhansonp. But I don't see a GEODE ticket for it.

✓ typos fixed
✓ a number of int stats became long stats
✓✓ final added in lots of places
𝙓 missing GEODE ticket reference

@echobravopapa
Copy link
Contributor

@mhansonp echoing @DonalEvans & @dschneider-pivotal on the GEODE jira etc... might be useful context to know the reason for the changes before I review...

@mhansonp
Copy link
Contributor Author

A few typos and some unintentionally included changes. Also, could the title of the PR please be changed to reference the associated GEODE ticket and be more descriptive?

No. I removed the GEODE- ticket stuff. The typos are a freebee. :)

@mhansonp
Copy link
Contributor Author

This looks good @mhansonp. But I don't see a GEODE ticket for it.

✓ typos fixed
✓ a number of int stats became long stats
✓✓ final added in lots of places
𝙓 missing GEODE ticket reference

I added a new improvement GEODE-9190 to cover this work.

@mhansonp mhansonp changed the title Bugfix/stats cleanup GEODE-9190 Cleanup of Stats Code Warnings Apr 22, 2021
@mhansonp
Copy link
Contributor Author

@mhansonp echoing @DonalEvans & @dschneider-pivotal on the GEODE jira etc... might be useful context to know the reason for the changes before I review...

Context: I need to do other stat adding and I kept coming across these stats files with many warnings. I finally got fed up and cleaned them up.

Bill
Bill approved these changes Apr 24, 2021
View reviewed changes
Copy link
Contributor

@Bill Bill left a comment

Choose a reason for hiding this comment

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

Approved!

nabarunnag
nabarunnag reviewed Apr 26, 2021
View reviewed changes
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBeanBridge.java
@@ -1392,7 +1392,7 @@ public String getRedundancyZone() {
}

int getRebalancesInProgress() {
return resourceManagerStats.getRebalancesInProgress();
return (int) resourceManagerStats.getRebalancesInProgress();
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue:
Possible loss of precision warning due to converting a long to int

Are there alternative to changing this MBean to be long rather than int.
Please do let me know if this is not possible on this thread and I will change the review to approved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this is an external interface. I have changed nothing regarding precision. I just moved the decision of precision loss down to the interface. The core of stats went away from ints a long time ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the int because I didn't want to change the external interface.

nabarunnag
nabarunnag requested changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@nabarunnag nabarunnag left a comment

Choose a reason for hiding this comment

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

#6347 (comment)

@mhansonp mhansonp merged commit 5c2fa04 into apache:develop Apr 30, 2021
@mhansonp mhansonp deleted the bugfix/StatsCleanup branch April 30, 2021 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bill Bill Bill approved these changes

@boglesby boglesby boglesby approved these changes

@kirklund kirklund kirklund approved these changes

@nabarunnag nabarunnag nabarunnag approved these changes

@gesterzhou gesterzhou gesterzhou approved these changes

@dschneider-pivotal dschneider-pivotal dschneider-pivotal approved these changes

@jinmeiliao jinmeiliao jinmeiliao approved these changes

@DonalEvans DonalEvans DonalEvans approved these changes

@mkevo mkevo mkevo approved these changes

@echobravopapa echobravopapa Awaiting requested review from echobravopapa

@jchen21 jchen21 Awaiting requested review from jchen21

@jdeppe-pivotal jdeppe-pivotal Awaiting requested review from jdeppe-pivotal

@pivotal-eshu pivotal-eshu Awaiting requested review from pivotal-eshu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone