You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/19 20:46:10 UTC
[GitHub] [geode] mhansonp opened a new pull request #6347: Bugfix/stats cleanup
mhansonp opened a new pull request #6347:
URL: https://github.com/apache/geode/pull/6347
I need to do other stat adding and I kept coming across these stats files with many warnings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617024351
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +425,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
+ f.createLongCounter("partitionedRegionClearLocalDuration",
Review comment:
I do believe I did with the latest checkin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-825222644
> 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. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617023693
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +425,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
+ f.createLongCounter("partitionedRegionClearLocalDuration",
+ "The time in nanoseconds partitioned region clear has been running for the region on this member",
+ "nanoseconds"),
+ f.createLongCounter("partitionedRegionClearTotalDuration",
+ "The time in nanoseconds partitioned region clear has been running for the region with this member as coordinator.",
+ "nanoseconds"),
Review comment:
They have been pulled with the latest checkin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6347: GEODE-9190 Cleanup of Stats Code Warnings
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-825224434
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: GEODE-9190 Cleanup of Stats Code Warnings
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r621720978
##########
File path: 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();
Review comment:
I left the int because I didn't want to change the external interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: GEODE-9190 Cleanup of Stats Code Warnings
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r621720883
##########
File path: 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();
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617070625
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/cache/PartitionedRegionStatsJUnitTest.java
##########
@@ -477,4 +480,27 @@ private long getMemBytes(PartitionedRegion pr) {
return bytes;
}
+
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp merged pull request #6347: GEODE-9190 Cleanup of Stats Code Warnings
Posted by GitBox <gi...@apache.org>.
mhansonp merged pull request #6347:
URL: https://github.com/apache/geode/pull/6347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617036255
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java
##########
@@ -1045,16 +1034,16 @@ public Set getAllRoles() {
private int lonerPort = 0;
- // private static final int CHARS_32KB = 16384;
+ // private static final long CHARS_32KB = 16384;
Review comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-825223813
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617052442
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
##########
@@ -680,10 +678,10 @@ public void handleManagerDeparture(InternalDistributedMember theId) {
}
/*
- * Returns an id of the thread in serialQueuedExecutorMap, thats mapped to the given seder.
+ * Returns an id of the thread in serialQueuedExecutorMap, that's mapped to the given seder.
*
*
- * @param createNew boolean flag to indicate whether to create a new id, if id doesnot exists.
+ * @param createNew boolean flag to indicate whether to create a new id, if id does not exists.
Review comment:
I will fix that spelling error too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r617023561
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
##########
@@ -680,10 +678,10 @@ public void handleManagerDeparture(InternalDistributedMember theId) {
}
/*
- * Returns an id of the thread in serialQueuedExecutorMap, thats mapped to the given seder.
+ * Returns an id of the thread in serialQueuedExecutorMap, that's mapped to the given seder.
Review comment:
Interesting. Intellij didn't note the other issue. I just fixed the one spelling error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-822833352
This pull request **introduces 1 alert** when merging 463a5a19b764052aeb211797b1ce91eef688a5f6 into 5f7abc97b0ad1626e359e36efc12734feb42f7b2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-3203d740e9eb68f903431d701a798410add54cbd)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r616862724
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +425,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
+ f.createLongCounter("partitionedRegionClearLocalDuration",
Review comment:
these should be removed since they are for PR clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-823667883
I've approved the code changes, but please also update the name of the PR to include the GEODE ticket associated with these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r616348827
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/cache/PartitionedRegionStatsJUnitTest.java
##########
@@ -477,4 +480,27 @@ private long getMemBytes(PartitionedRegion pr) {
return bytes;
}
+
Review comment:
This section may need to be struck as and stuff related to clear came from GEODE-7665
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r616833221
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
##########
@@ -680,10 +678,10 @@ public void handleManagerDeparture(InternalDistributedMember theId) {
}
/*
- * Returns an id of the thread in serialQueuedExecutorMap, thats mapped to the given seder.
+ * Returns an id of the thread in serialQueuedExecutorMap, that's mapped to the given seder.
Review comment:
Typo here, should be "sender". Also, the comma should probably be removed.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +425,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
+ f.createLongCounter("partitionedRegionClearLocalDuration",
+ "The time in nanoseconds partitioned region clear has been running for the region on this member",
+ "nanoseconds"),
+ f.createLongCounter("partitionedRegionClearTotalDuration",
+ "The time in nanoseconds partitioned region clear has been running for the region with this member as coordinator.",
+ "nanoseconds"),
Review comment:
These stats are newly added with this PR, which seems like an accidental inclusion.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterOperationExecutors.java
##########
@@ -680,10 +678,10 @@ public void handleManagerDeparture(InternalDistributedMember theId) {
}
/*
- * Returns an id of the thread in serialQueuedExecutorMap, thats mapped to the given seder.
+ * Returns an id of the thread in serialQueuedExecutorMap, that's mapped to the given seder.
*
*
- * @param createNew boolean flag to indicate whether to create a new id, if id doesnot exists.
+ * @param createNew boolean flag to indicate whether to create a new id, if id does not exists.
Review comment:
Typo here, should be "exist".
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LonerDistributionManager.java
##########
@@ -1045,16 +1034,16 @@ public Set getAllRoles() {
private int lonerPort = 0;
- // private static final int CHARS_32KB = 16384;
+ // private static final long CHARS_32KB = 16384;
Review comment:
This commented out line can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-822796710
This pull request **introduces 1 alert** when merging 28351899f7649519afd7f463478687ffcf9a5194 into 5f7abc97b0ad1626e359e36efc12734feb42f7b2 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-4d1f5c969f7a892860bb0dfed7a62fcaab9e281f)
**new alerts:**
* 1 for Result of multiplication cast to wider type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r616346209
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +429,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
Review comment:
Note: some stats got moved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r616346209
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionStats.java
##########
@@ -425,7 +429,13 @@
false),
f.createLongGauge("localMaxMemory",
- "local max memory in bytes for this region on this member", "bytes")
+ "local max memory in bytes for this region on this member", "bytes"),
Review comment:
Note: some stats got moved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] echobravopapa commented on pull request #6347: Bugfix/stats cleanup
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on pull request #6347:
URL: https://github.com/apache/geode/pull/6347#issuecomment-825221918
@mhansonp echoing @DonalEvans & @dschneider-pivotal on the GEODE jira etc... might be useful context to know the reason for the changes before I review...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #6347: GEODE-9190 Cleanup of Stats Code Warnings
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6347:
URL: https://github.com/apache/geode/pull/6347#discussion_r620582045
##########
File path: 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();
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org