pivotal-jbarrett commented on a change in pull request #6037:
URL: https://github.com/apache/geode/pull/6037#discussion_r582368198
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -1231,23 +1197,16 @@ public void shutdownMessageReceived(ID id, String
reason) {
if (logger.isDebugEnabled()) {
logger.debug("Membership: recording shutdown status of {}", id);
}
- synchronized (this.shutdownMembers) {
- this.shutdownMembers.put(id, id);
- services.getHealthMonitor()
- .memberShutdown(id, reason);
- services.getJoinLeave().memberShutdown(id, reason);
- }
+ this.shutdownMembers.add(id);
+ services.getJoinLeave().memberShutdown(id, reason);
}
@Override
public Set<ID> getMembersNotShuttingDown() {
- latestViewReadLock.lock();
- try {
- return latestView.getMembers().stream().filter(id ->
!shutdownMembers.containsKey(id))
+ synchronized (shutdownMembers) {
Review comment:
If the `shutdownMembers` list isn't changing frequently, like every ms,
then repeatedly recreating the same set is just adding pressure to the GC and
CPU time all while under a synchronized lock for no real gain. I assume this
`shutdownMembers` list is updated with the member id of the member that has
said it is or has gone down, which isn't a frequent event in production. Seems
like we should be optimizing this method to be returning a rather static list
of members known not to be shutting down even if that means the infrequent
setting of a shutting down member takes a bit more time to handle, by copying
lists and invalidating or updating the not shutdown servers list.
At the very least the addition of this synchronized block is going to have a
noticeable impact on the p2p messaging at scale since all messages will be
serialized through this lock. If we deem refactoring out the list creation is
out of scope for this PR then using a read/write lock here should be in scope
as a first step.
----------------------------------------------------------------
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:
[email protected]
