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/03/30 09:59:09 UTC
[GitHub] [geode] mkevo opened a new pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
mkevo opened a new pull request #6225:
URL: https://github.com/apache/geode/pull/6225
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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] mkevo commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r609389175
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
In the description of this attribute state the following:
`The current number of nodes in this distributed system visible to this member.`
What are nodes there, both locators and servers or just servers?
Also, should the current server be visible to itself?
If we go with @jinmeiliao proposal we should change the description to be more clear.
`The current number of servers in this distributed system visible to this member.`
--
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] jinmeiliao commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r608917610
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
From the original code, it looks like it doesn't think a locator should be included in the count, so, shouldn't the correct count should be:
```
Locator 1: 2 (count Server 1 and Server 2)
Locator 2: 2 (count Server 1 and Server 2)
Server 1: 2 (count Server 1 and Server 2)
Server 2: 2 (count Server 1 and Server 2)
```
instead?
--
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] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-823809483
@kirklund , can you re-review it, please?
--
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] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r617964050
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
I'm marking this as resolved because I think developers who have worked on geode-membership should review it instead of me.
--
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] mkevo commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r605461051
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
I don't want to change "nodes" to show the number of "servers".
How it works by now?
If we have 2 locators and 2 servers the VisibleNodes attribute has the following values:
```
Locator 1: 2 (count Server 1 and Server 2)
Locator 2: 2 (count Server 1 and Server 2)
Server 1: 3 (count Server 2, and twice Server 1)
Server 2: 3 (count Server 1, and twice Server 2)
```
With my changes it will count every member without itself:
```
Locator 1: 3 (count Locator 2, Server 1, and Server 2)
Locator 2: 3 (count Locator 1, Server 1, and Server 2)
Server 1: 3 (count Locator 1, Locator 2, and Server 2)
Server 2: 3 (count Locator 1, Locator 2, and Server 1)
```
What do you mean by the number of "peer-to-peer" processes in the cluster?
Is that include both locators and servers?
--
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] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r609909199
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
I understand the purpose of the PR better now. However, the PR is still trying to redefine the "nodes" statistic in DistributionStats which is not correct. You should instead add a new attribute to MemberMXBean that returns the count of OTHER members in the cluster.
Here is the definition of the "nodes" statistic:
```
final String nodesDesc = "The current number of nodes in this distributed system.";
```
So, if you have 2 servers and 2 locators, "nodes" should be 4 within all 4 JVMs.
The earliest version of GemFire had locators which were not members (ie not nodes), but locators now are definitely members (ie nodes). The next type of locators in GemFire were members (ie nodes) but without caches. As of GemFire 7.0, all locators have a cache. As of Geode 1.0, all locators are members with caches. The methods to create that earliest type of locator still exists but are deprecated... so deprecated internal methods is now the only way to create the thin locators that are not peer-to-peer members of the cluster.
Changing the attribute of an MXBean is _different_ from changing the meaning of the statistic. The statistic is not supposed to ignore locators.
--
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] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-917859079
> looks like this may not be moving forward, can it be closed?
I still waiting for an answer on the geode dev mail list. So this cannot be closed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-933932988
> > looks like this may not be moving forward, can it be closed?
>
> I still waiting for an answer on the geode dev mail list. So this cannot be closed.
@mkevo since this is dragging out and you didn't get an answer on dev, I would write up a simple proposal on the wiki (similar to the other proposals) and start a review of that on the dev list.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-934086362
> > > looks like this may not be moving forward, can it be closed?
> >
> >
> > I still waiting for an answer on the geode dev mail list. So this cannot be closed.
>
> @mkevo since this is dragging out and you didn't get an answer on dev, I would write up a simple proposal on the wiki (similar to the other proposals) and start a review of that on the dev list.
Thanks @kirklund!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-1008659844
> > > > looks like this may not be moving forward, can it be closed?
> > >
> > >
> > > I still waiting for an answer on the geode dev mail list. So this cannot be closed.
> >
> >
> > @mkevo since this is dragging out and you didn't get an answer on dev, I would write up a simple proposal on the wiki (similar to the other proposals) and start a review of that on the dev list.
>
> Hi @kirklund, What is the status of this activity?
Hi @kirklund, any news on this?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-998583053
> > > looks like this may not be moving forward, can it be closed?
> >
> >
> > I still waiting for an answer on the geode dev mail list. So this cannot be closed.
>
> @mkevo since this is dragging out and you didn't get an answer on dev, I would write up a simple proposal on the wiki (similar to the other proposals) and start a review of that on the dev list.
Hi @kirklund,
What is the status of this activity?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r605267417
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
I think you're trying to change "nodes" to show the number of "servers" in the cluster but it's a different concept. "nodes" is a statistic in DistributionStats that reflects the number of peer-to-peer processes in the cluster, and we really need this statistic to remain as is.
I recommend adding one or more new attribute(s) to MemberMXBean or DistributedSystemMXBean which reflect the number of "non-locator members" or "servers". There are several related operations on DistributedSystemMXBean such listLocators(), listMembers(), listServers(). All three of these are "nodes".
listMembers() should show you all of the locators and servers (ie, any peer-to-peer process) in the cluster.
listLocators() should should you all of the locators only.
listServers() should show you all of the servers only.
There is a subtle difference between Attributes and Operations in JMX. You probably want an Attribute in this case so that it can be monitored with JMX APIs. But I'll leave further planning to you.
--
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 #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-859943953
@kirklund I noticed your comments on the JIRA ticket, do you still think this warrants dev list discussion and possibly even an RFC? @mkevo did you see his comments on the ticket?
--
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] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-861427086
Hi @echobravopapa, I think that for adding a new attribute we need to start the discussion on the dev list or write an RFC. But for this case, we just changed the existing attribute to print the correct values.
Some time ago, when this attribute was added, the locators weren't members/nodes, then it is changed and locators became members/nodes without a cache. From GemFire 7.0, all locators also have a cache and they are members/nodes and we should change this attribute to get the correct value for all visible nodes.
--
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] onichols-pivotal commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-917296879
looks like this may not be moving forward, can it be closed?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mkevo commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r618159171
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -350,13 +350,13 @@ static ClusterDistributionManager create(InternalDistributedSystem system,
}
}
}
+ } else {
+ distributionManager.addNewMember(id); // add ourselves
Review comment:
In the method, _create_(ClusterDistributionManager.java) it adds itself twice(ClusterDistributionManager constructor and there again). So it increments this attribute for both _addNewMember()_ call.
--
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] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r617959313
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -350,13 +350,13 @@ static ClusterDistributionManager create(InternalDistributedSystem system,
}
}
}
+ } else {
+ distributionManager.addNewMember(id); // add ourselves
Review comment:
This block of code needs to addNewMember even when the name is "". Why does it need to move it into an else-block?
If you start a Geode process without using `start locator` or `start server`, you can do so without a `name` but it's still a new member.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1793,9 +1793,7 @@ private String prettifyReason(String r) {
*/
private void handleManagerStartup(InternalDistributedMember theId) {
// Note test is under membersLock
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(1);
- }
+ stats.incNodes(1);
Review comment:
I think @Bill @bschuchardt or @dschneider-pivotal should review this change instead of me.
--
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] mkevo commented on pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #6225:
URL: https://github.com/apache/geode/pull/6225#issuecomment-1073648416
@Bill, can you please add your review regarding your comment on [RFC](https://cwiki.apache.org/confluence/display/GEODE/Introducing+VisibleMembers+attribute)?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r609909199
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
removeHostedLocators(theId);
redundancyZones.remove(theId);
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
+ stats.incNodes(-1);
+
Review comment:
I understand the purpose of the PR better now. However, the PR is still trying to redefine the "nodes" statistic in DistributionStats which is not correct. You should instead add a new attribute to MemberMXBean that returns the count of OTHER members in the cluster.
Here is the definition of the "nodes" statistic:
```
final String nodesDesc = "The current number of nodes in this distributed system.";
```
So, if you have 2 servers and 2 locators, "nodes" should be 4 within all 4 JVMs.
The earliest version of GemFire had locators which were not members (ie not nodes), but locators now are definitely members (ie nodes). The next type of locators in GemFire were members (ie nodes) but without caches. As of GemFire 7.0, all locators also have a cache. So in every version of Geode, all locators are members with caches. The methods to create that earliest type of locator still exists but are deprecated... so deprecated internal methods is now the only way to create the thin locators that are not peer-to-peer members of the cluster.
Changing the attribute of an MXBean is _different_ from changing the meaning of the statistic. The statistic is not supposed to ignore locators.
--
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