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 2020/11/30 18:56:57 UTC
[GitHub] [geode] jhutchison opened a new pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
jhutchison opened a new pull request #5778:
URL: https://github.com/apache/geode/pull/5778
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:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] 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] jhutchison commented on pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5778:
URL: https://github.com/apache/geode/pull/5778#issuecomment-738976840
@kirklund thanks for input- we're looking into these
----------------------------------------------------------------
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] jhutchison commented on a change in pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r542554633
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
ok, maybe it makes most sense to wait wait until 5278 is pull merged, and then adapt off that. @kirklund, Are you expecting to have time to get that through in the near future?
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r537737196
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -18,35 +18,34 @@
import org.apache.logging.log4j.Logger;
+import org.apache.geode.CancelCriterion;
import org.apache.geode.cache.EntryEvent;
import org.apache.geode.cache.util.CacheListenerAdapter;
+import org.apache.geode.internal.cache.InternalCacheForClientAccess;
+import org.apache.geode.internal.util.concurrent.StoppableCountDownLatch;
import org.apache.geode.logging.internal.log4j.api.LogService;
/**
* This listener is attached to the Monitoring Region to receive any addition or deletion of MBEans
- *
- * It updates the last refreshed time of proxy once it gets the update request from the Managed Node
- *
- *
+ * <p>
+ * It updates the last refreshed time of proxy once it gets the update request from the Managed
+ * Node
*/
public class ManagementCacheListener extends CacheListenerAdapter<String, Object> {
private static final Logger logger = LogService.getLogger();
+ private final MBeanProxyFactory proxyHelper;
+ private final StoppableCountDownLatch readyForEvents;
- private MBeanProxyFactory proxyHelper;
-
- private volatile boolean readyForEvents;
-
- public ManagementCacheListener(MBeanProxyFactory proxyHelper) {
+ public ManagementCacheListener(MBeanProxyFactory proxyHelper,
+ InternalCacheForClientAccess cache) {
this.proxyHelper = proxyHelper;
- this.readyForEvents = false;
+ this.readyForEvents = new StoppableCountDownLatch(new CacheListenerCancelCriterion(cache), 1);
}
@Override
public void afterCreate(EntryEvent<String, Object> event) {
- if (!readyForEvents) {
- return;
- }
+// blockUntilReady();
Review comment:
new dead code.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -459,35 +481,18 @@ public boolean hasOwnStats() {
if (!proxyNotificationRegionCreated) {
// Destroy the proxy region if proxy notification region is not created
proxyMonitoringRegion.localDestroyRegion();
+ if (logger.isDebugEnabled()) {
+ logger.debug(" Failed to create proxy notification region,"
+ + "destroying proxy monitoring region with name : {}",
+ proxyMonitoringRegion.getName());
+ }
}
}
- if (logger.isDebugEnabled()) {
- logger.debug("Management Region created with Name : {}",
- proxyMonitoringRegion.getName());
- logger.debug("Notification Region created with Name : {}",
- proxyNotificationRegion.getName());
- }
-
// Only the exception case would have destroyed the proxy
// regions. We can safely proceed here.
- repo.putEntryInMonitoringRegionMap(member, proxyMonitoringRegion);
repo.putEntryInNotifRegionMap(member, proxyNotificationRegion);
- try {
- if (!running) {
- return;
- }
- proxyFactory.createAllProxies(member, proxyMonitoringRegion);
-
- managementCacheListener.markReady();
- notifListener.markReady();
- } catch (Exception e) {
- if (logger.isDebugEnabled()) {
- logger.debug("Error During GII Proxy creation", e);
- }
-
- throw new ManagementException(e);
- }
+// notifListener.markReady(); //@todo was dependent on proxies for monitoring region being created before we moved it
Review comment:
new dead code. please remove... No new todo's please submit a bug.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/NotificationCacheListener.java
##########
@@ -15,100 +15,76 @@
package org.apache.geode.management.internal;
+
import javax.management.Notification;
-import org.apache.geode.cache.CacheListener;
+import org.apache.geode.CancelCriterion;
import org.apache.geode.cache.EntryEvent;
-import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.util.CacheListenerAdapter;
+import org.apache.geode.internal.cache.InternalCacheForClientAccess;
+import org.apache.geode.internal.util.concurrent.StoppableCountDownLatch;
/**
* This listener will be attached to each notification region corresponding to a member
- *
*/
-public class NotificationCacheListener implements CacheListener<NotificationKey, Notification> {
-
- /**
- * For the
- */
- private NotificationHubClient notifClient;
+public class NotificationCacheListener extends CacheListenerAdapter<NotificationKey, Notification> {
- private volatile boolean readyForEvents;
+ private final NotificationHubClient notifClient;
+ private final StoppableCountDownLatch readyForEvents;
- public NotificationCacheListener(MBeanProxyFactory proxyHelper) {
-
- notifClient = new NotificationHubClient(proxyHelper);
- this.readyForEvents = false;
+ public NotificationCacheListener(InternalCacheForClientAccess cache,
+ MBeanProxyFactory proxyHelper) {
+ notifClient =
+ new NotificationHubClient(proxyHelper);
+ this.readyForEvents =
+ new StoppableCountDownLatch(new CacheListenerCancelCriterion(cache), 1);
}
@Override
public void afterCreate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
- }
-
- @Override
- public void afterDestroy(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
}
@Override
- public void afterInvalidate(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionClear(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionCreate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
+ public void afterUpdate(EntryEvent<NotificationKey, Notification> event) {
+ blockUntilReady();
+ notifClient.sendNotification(event);
}
- @Override
- public void afterRegionDestroy(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
+ private void blockUntilReady() {
Review comment:
unused method
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -104,13 +102,41 @@ public void afterUpdate(EntryEvent<String, Object> event) {
if (logger.isDebugEnabled()) {
logger.debug("Proxy Update failed for {} with exception {}", objectName, e.getMessage(), e);
}
-
}
+ }
+ private void blockUntilReady() {
Review comment:
unused method
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/NotificationCacheListener.java
##########
@@ -15,100 +15,76 @@
package org.apache.geode.management.internal;
+
import javax.management.Notification;
-import org.apache.geode.cache.CacheListener;
+import org.apache.geode.CancelCriterion;
import org.apache.geode.cache.EntryEvent;
-import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.util.CacheListenerAdapter;
+import org.apache.geode.internal.cache.InternalCacheForClientAccess;
+import org.apache.geode.internal.util.concurrent.StoppableCountDownLatch;
/**
* This listener will be attached to each notification region corresponding to a member
- *
*/
-public class NotificationCacheListener implements CacheListener<NotificationKey, Notification> {
-
- /**
- * For the
- */
- private NotificationHubClient notifClient;
+public class NotificationCacheListener extends CacheListenerAdapter<NotificationKey, Notification> {
- private volatile boolean readyForEvents;
+ private final NotificationHubClient notifClient;
+ private final StoppableCountDownLatch readyForEvents;
- public NotificationCacheListener(MBeanProxyFactory proxyHelper) {
-
- notifClient = new NotificationHubClient(proxyHelper);
- this.readyForEvents = false;
+ public NotificationCacheListener(InternalCacheForClientAccess cache,
+ MBeanProxyFactory proxyHelper) {
+ notifClient =
+ new NotificationHubClient(proxyHelper);
+ this.readyForEvents =
+ new StoppableCountDownLatch(new CacheListenerCancelCriterion(cache), 1);
}
@Override
public void afterCreate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
- }
-
- @Override
- public void afterDestroy(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
}
@Override
- public void afterInvalidate(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionClear(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionCreate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
+ public void afterUpdate(EntryEvent<NotificationKey, Notification> event) {
+ blockUntilReady();
+ notifClient.sendNotification(event);
}
- @Override
- public void afterRegionDestroy(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
+ throw new RuntimeException(e);
+ }
}
- @Override
- public void afterRegionInvalidate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
+ void markReady() {
Review comment:
unused.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -76,16 +75,15 @@ public void afterDestroy(EntryEvent<String, Object> event) {
e);
}
}
-
}
@Override
public void afterUpdate(EntryEvent<String, Object> event) {
+// blockUntilReady();
Review comment:
new dead code.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -59,11 +58,11 @@ public void afterCreate(EntryEvent<String, Object> event) {
logger.debug("Proxy Create failed for {} with exception {}", objectName, e.getMessage(), e);
}
}
-
}
@Override
public void afterDestroy(EntryEvent<String, Object> event) {
+// blockUntilReady();
Review comment:
new dead code.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -407,11 +406,42 @@ public boolean hasOwnStats() {
monitorFactory.setDataPolicy(DataPolicy.REPLICATE);
monitorFactory.setConcurrencyChecksEnabled(false);
ManagementCacheListener managementCacheListener =
- new ManagementCacheListener(proxyFactory);
+ new ManagementCacheListener(proxyFactory, cache);
monitorFactory.addCacheListener(managementCacheListener);
monitorFactory.setIsUsedForMetaRegion(true);
monitorFactory.setCachePerfStatsHolder(monitoringRegionStats);
+ Region<String, Object> proxyMonitoringRegion;
+
+ if (!running) { return; }
+
+ try {
+ proxyMonitoringRegion = monitorFactory.create(monitoringRegionName);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Monitoring Region created with Name : {}",
+ proxyMonitoringRegion.getName());
+ }
+ } catch (TimeoutException | RegionExistsException e) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Error During Internal Region creation", e);
+ }
+ throw new ManagementException(e);
+ }
+
+ repo.putEntryInMonitoringRegionMap(member, proxyMonitoringRegion);
+ if (!running) { return; }
+
+ try {
+ proxyFactory.createAllProxies(member, proxyMonitoringRegion);
+// managementCacheListener.markReady();
Review comment:
new dead code
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -104,13 +102,41 @@ public void afterUpdate(EntryEvent<String, Object> event) {
if (logger.isDebugEnabled()) {
logger.debug("Proxy Update failed for {} with exception {}", objectName, e.getMessage(), e);
}
-
}
+ }
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
+ throw new RuntimeException(e);
+ }
}
void markReady() {
Review comment:
unused method
----------------------------------------------------------------
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] jhutchison commented on pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5778:
URL: https://github.com/apache/geode/pull/5778#issuecomment-740182622
> It seems like there is some cleanup that needs to be done based on what my editor is telling me. Please take a look at these comments. if you disagree, please disregard the comments.
Thanks Mark- this was just a quick push to experiment with some stuff-- will make sure none of these are in final code :)
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r542625730
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
@jdeppe-pivotal hmm, well now you have me second-guessing the need for CancelCriterion. Maybe we can get another review before we make more changes on this PR. EDIT: if we don't don't need the Cache's CancelCriterion, then we don't need to use any Stoppable* classes either.
----------------------------------------------------------------
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] jhutchison commented on a change in pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r542554633
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
ok, maybe it makes most sense to wait wait until 5278 is pull merged, and then adapt off that. Are you expecting to have time to get that through in the near future?
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r541313975
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/NotificationCacheListener.java
##########
@@ -14,101 +14,51 @@
*/
package org.apache.geode.management.internal;
-
import javax.management.Notification;
-import org.apache.geode.cache.CacheListener;
+import org.apache.geode.CancelCriterion;
import org.apache.geode.cache.EntryEvent;
-import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.util.CacheListenerAdapter;
+import org.apache.geode.internal.util.concurrent.StoppableCountDownLatch;
/**
* This listener will be attached to each notification region corresponding to a member
- *
*/
-public class NotificationCacheListener implements CacheListener<NotificationKey, Notification> {
-
- /**
- * For the
- */
- private NotificationHubClient notifClient;
+public class NotificationCacheListener extends CacheListenerAdapter<NotificationKey, Notification> {
- private volatile boolean readyForEvents;
+ private final NotificationHubClient notifClient;
+ private final StoppableCountDownLatch readyForEvents;
- public NotificationCacheListener(MBeanProxyFactory proxyHelper) {
-
- notifClient = new NotificationHubClient(proxyHelper);
- this.readyForEvents = false;
+ public NotificationCacheListener(MBeanProxyFactory proxyHelper, CancelCriterion cancelCriterion) {
+ notifClient =
+ new NotificationHubClient(proxyHelper);
+ this.readyForEvents =
+ new StoppableCountDownLatch(cancelCriterion, 1);
}
@Override
public void afterCreate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
- }
-
- @Override
- public void afterDestroy(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterInvalidate(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionClear(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionCreate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionDestroy(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionInvalidate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionLive(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
}
@Override
public void afterUpdate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
}
- @Override
- public void close() {
- // TODO Auto-generated method stub
-
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
```
try {
readyForEvents.await();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
```
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -104,13 +101,19 @@ public void afterUpdate(EntryEvent<String, Object> event) {
if (logger.isDebugEnabled()) {
logger.debug("Proxy Update failed for {} with exception {}", objectName, e.getMessage(), e);
}
-
}
+ }
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
`Thread.interrupted()` only checks to see if the interrupt bit is set (returns boolean). You want to use `Thread.currentThread().interrupt()` like this:
```
try {
readyForEvents.await();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
```
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
Creating a new CancelCriterion instance isn't really what is needed here. You need to `getCancelCriterion` from either the Cache or the DistributedSystem and pass that into the constructor of the classes that are using it. Then if the DS or Cache are closing, the CancelCriterion will cause the FederatingManager to cancel with the cause being the Cache or DS being closed.
If you look at #5728 (my PR with changes to FederatingManager), you'll see that the constructors are now passing in an instance of `CancelCriterion` which comes from `InternalDistributedSystem`:
```
FederatingManager(ManagementResourceRepo repo, InternalDistributedSystem system,
SystemManagementService service, InternalCache cache, StatisticsFactory statisticsFactory,
StatisticsClock statisticsClock, MBeanProxyFactory proxyFactory, MemberMessenger messenger,
Supplier<ExecutorService> executorServiceSupplier) {
this(repo, system, service, cache, statisticsFactory, statisticsClock, proxyFactory, messenger,
system.getCancelCriterion(), executorServiceSupplier);
}
@VisibleForTesting
FederatingManager(ManagementResourceRepo repo, InternalDistributedSystem system,
SystemManagementService service, InternalCache cache, StatisticsFactory statisticsFactory,
StatisticsClock statisticsClock, MBeanProxyFactory proxyFactory, MemberMessenger messenger,
CancelCriterion cancelCriterion, ExecutorService executorService) {
this(repo, system, service, cache, statisticsFactory, statisticsClock, proxyFactory, messenger,
cancelCriterion, () -> executorService);
}
@VisibleForTesting
FederatingManager(ManagementResourceRepo repo, InternalDistributedSystem system,
SystemManagementService service, InternalCache cache, StatisticsFactory statisticsFactory,
StatisticsClock statisticsClock, MBeanProxyFactory proxyFactory, MemberMessenger messenger,
CancelCriterion cancelCriterion, Supplier<ExecutorService> executorServiceSupplier) {
super(repo, system, cache, statisticsFactory, statisticsClock);
this.service = service;
this.proxyFactory = proxyFactory;
this.messenger = messenger;
this.executorServiceSupplier = executorServiceSupplier;
lifecycleLock = new StoppableReentrantLock(cancelCriterion);
}
```
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r542625730
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
@jdeppe-pivotal hmm, well now you have me second-guessing the need for CancelCriterion. Maybe we can get another review before we make more changes on this PR.
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r541332662
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -104,13 +101,19 @@ public void afterUpdate(EntryEvent<String, Object> event) {
if (logger.isDebugEnabled()) {
logger.debug("Proxy Update failed for {} with exception {}", objectName, e.getMessage(), e);
}
-
}
+ }
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
Another option which might be more correct and it matches most of the Geode code more closely:
```
boolean interrupted = false;
try {
readyForEvents.await();
} catch (InterruptedException e) {
interrupted = true;
cancelCriterion.checkCancelInProgress(ie);
throw new RuntimeException(e);
} finally {
if (interrupted) {
Thread.currentThread().interrupt();
}
}
```
The effect is that if the thread is interrupted, then we prefer Cache closed exception or DistributedSystem disconnected exception first. If neither is occurring then we throw new RuntimeException as a last resort. And then finally we reset the thread interrupt flag since we didn't actually handle the interrupt locally here by halting the Runnable that the thread is executing. The code is basically passing along a runtime exception and interrupt flag back to the calling code to let it deal with both.
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r542621525
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
I hope to have #5728 ready to merge today at the soonest or in a day or two.
----------------------------------------------------------------
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] jhutchison commented on pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5778:
URL: https://github.com/apache/geode/pull/5778#issuecomment-791611415
code to be resubmitted in a future PR
----------------------------------------------------------------
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] jhutchison closed pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jhutchison closed pull request #5778:
URL: https://github.com/apache/geode/pull/5778
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r541332662
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -104,13 +101,19 @@ public void afterUpdate(EntryEvent<String, Object> event) {
if (logger.isDebugEnabled()) {
logger.debug("Proxy Update failed for {} with exception {}", objectName, e.getMessage(), e);
}
-
}
+ }
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ Thread.interrupted();
Review comment:
Another option which might be more correct and it matches most of the Geode code more closely:
```
boolean interrupted = false;
try {
readyForEvents.await();
} catch (InterruptedException e) {
interrupted = true;
getCancelCriterion().checkCancelInProgress(ie);
throw new RuntimeException(e);
} finally {
if (interrupted) {
Thread.currentThread().interrupt();
}
}
```
The effect is that if the thread is interrupted, then we prefer Cache closed exception or DistributedSystem disconnected exception first. If neither is occurring then we throw new RuntimeException as a last resort. And then finally we reset the thread interrupt flag since we didn't actually handle the interrupt locally here by halting the Runnable that the thread is executing. The code is basically passing along a runtime exception and interrupt flag back to the calling code to let it deal with both.
----------------------------------------------------------------
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] jdeppe-pivotal commented on a change in pull request #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r541439516
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java
##########
@@ -380,8 +384,10 @@ void addMemberArtifacts(InternalDistributedMember member) {
return;
}
- try {
+ FederatingManagerCancelCriterion cancelCriterion =
Review comment:
I'm really not familiar with the usage of `CancelCriterion` so perhaps my understanding of what's required here is a bit naive... My understanding was simply that we didn't want to indefinitely block any threads `await`ing in either of the two listeners in question here. That situation would arise if something bad happened in the `FederatingManager` before it could get to calling `readyForEvents`. Hence the use of the `StoppableCountdownLatch`.
Since what's calling into these listeners is a callback using an asynchronous thread, I don't understand why it is the responsibility of these listeners to go through all of this ceremony to tell some random component that the cache is closing. Surely that component should have its own means of determining that the cache is closing? Why is it not enough for the thread to be released and throw an exception?
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5778:
URL: https://github.com/apache/geode/pull/5778#issuecomment-739077973
This pull request **introduces 1 alert** when merging b178b9fd0b381c472f0e5f334f667b22e51a2999 into 9ccef088ed5df32afaae1ceb7725be561544716d - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2e6f94cbcfe1cee91e9912b731f85a4035c491e3)
**new alerts:**
* 1 for Missing space in string literal
----------------------------------------------------------------
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 #5778: GEODE-7739: Address race condition in FederatingManager during listener creation
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #5778:
URL: https://github.com/apache/geode/pull/5778#discussion_r532882325
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -109,8 +108,15 @@ public void afterUpdate(EntryEvent<String, Object> event) {
}
- void markReady() {
- readyForEvents = true;
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ // ignored
Review comment:
Anytime you catch an InterruptedException, you should reset the interrupt bit unless the thread is locally owned and it will actually terminate when interrupted:
```
try {
readyForEvents.await();
} catch (InterruptedException e) {
Thread.interrupt();
// recommended: throw new RunnableException(e); -- use an appropriate GemFire exception here
}
```
@upthewaterspout wrote an article but about this but I'm not sure where it is.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/NotificationCacheListener.java
##########
@@ -15,100 +15,47 @@
package org.apache.geode.management.internal;
+import java.util.concurrent.CountDownLatch;
+
import javax.management.Notification;
-import org.apache.geode.cache.CacheListener;
import org.apache.geode.cache.EntryEvent;
-import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.util.CacheListenerAdapter;
/**
* This listener will be attached to each notification region corresponding to a member
- *
*/
-public class NotificationCacheListener implements CacheListener<NotificationKey, Notification> {
-
- /**
- * For the
- */
- private NotificationHubClient notifClient;
+public class NotificationCacheListener extends CacheListenerAdapter<NotificationKey, Notification> {
- private volatile boolean readyForEvents;
+ private final NotificationHubClient notifClient;
+ private final CountDownLatch readyForEvents;
public NotificationCacheListener(MBeanProxyFactory proxyHelper) {
-
notifClient = new NotificationHubClient(proxyHelper);
- this.readyForEvents = false;
-
+ this.readyForEvents = new CountDownLatch(1);
}
@Override
public void afterCreate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
- }
-
- @Override
- public void afterDestroy(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterInvalidate(EntryEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionClear(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionCreate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionDestroy(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionInvalidate(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
- }
-
- @Override
- public void afterRegionLive(RegionEvent<NotificationKey, Notification> event) {
- // TODO Auto-generated method stub
-
}
@Override
public void afterUpdate(EntryEvent<NotificationKey, Notification> event) {
- if (!readyForEvents) {
- return;
- }
+ blockUntilReady();
notifClient.sendNotification(event);
-
}
- @Override
- public void close() {
- // TODO Auto-generated method stub
-
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
Review comment:
Same problems as described for ManagementCacheListener apply here.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -109,8 +108,15 @@ public void afterUpdate(EntryEvent<String, Object> event) {
}
- void markReady() {
- readyForEvents = true;
+ private void blockUntilReady() {
+ try {
+ readyForEvents.await();
+ } catch (InterruptedException e) {
+ // ignored
Review comment:
Also: If you don't rethrow the InterruptedException within a runtime exception, then this code probably needs to perform some sort of loop that checks a CancelCriterion. Grepping 'catch (InterruptedException' on the Geode product code (not tests) should give some good examples.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/ManagementCacheListener.java
##########
@@ -33,20 +35,18 @@
private static final Logger logger = LogService.getLogger();
- private MBeanProxyFactory proxyHelper;
+ private final MBeanProxyFactory proxyHelper;
- private volatile boolean readyForEvents;
+ private final CountDownLatch readyForEvents;
Review comment:
The latch should probably be an instance of StoppableCountDownLatch so that it will abort (by throwing) if the cache is closed while threads are waiting on the latch. Otherwise, this has the potential to hang.
----------------------------------------------------------------
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