Bill commented on a change in pull request #6827:
URL: https://github.com/apache/geode/pull/6827#discussion_r715846966
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipListener.java
##########
@@ -71,4 +71,7 @@ void memberSuspect(ID suspect, ID whoSuspected,
*/
void saveConfig();
+ default Throwable setShutdownCause(Throwable t) {
Review comment:
Need doc comment on this method. Is it meant to notify a listener of the
decision (by membership) as to the reason for a shutdown? If so then why is the
method returning a value? I notice the implementation of this method in
`ClusterDistributionManager.DMListener` returns a value different from the
value sent in. Is membership meant to do something with that result? What is
the contract?
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -1797,19 +1815,9 @@ public void forceDisconnect(final String reason) {
}
if (this.isReconnectingDS()) {
- logger.info("Reconnecting system failed to connect");
- uncleanShutdown(reason,
- new MemberDisconnectedException("reconnecting system failed to
connect"));
- return;
- }
-
- try {
- listener.saveConfig();
- } finally {
- new LoggingThread("DisconnectThread", false, () -> {
- lifecycleListener.forcedDisconnect();
- uncleanShutdown(reason, shutdownCause);
- }).start();
+ uncleanShutdownReconnectingDS(reason, shutdownCause);
Review comment:
Around line 1808 we do this:
```java
services.setShutdownCause(shutdownCause);
```
But we're going to do that in both branches of the conditional on line 1817.
In both cases we end up in `GMSMembership.uncleanShutdown()`.
Not only is it redundant to make the call in this method and in that other
one, it's also wrong since in `uncleanShutdownReconnectingDS()` we're going to
_change_ the exception that we use to notify listeners. So listeners will be
notified of a different exception than the one we just set via
`setShutdownCause(shutdownCause)`.
Recommend eliminating the call to `setShutdownCause(shutdownCause)` around
line 1808 of this method.
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -2335,6 +2330,16 @@ public void membershipFailure(String reason, Throwable
t) {
}
}
+ @Override
+ public Throwable setShutdownCause(Throwable cause) {
+ if (cause != null && !(cause instanceof ForcedDisconnectException)) {
Review comment:
Reinterpreting the `cause` sent to this listener method breaks the
contract. The listener is a recipient of information (in this case, from
membership). What if the reason for shutdown was not a forced disconnect?
Also wrong, is calling this listener method from the `membershipFailure()`
method above. This method is a callback that should only be invoked by
membership.
In fact before this method is invoked from `membershipFailure()` it has
already been invoked by membership.
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -1775,6 +1775,24 @@ public void memberSuspected(ID initiator, ID suspect,
String reason) {
handleOrDeferSuspect(s);
}
+ void uncleanShutdownReconnectingDS(String reason, Exception shutdownCause)
{
+ logger.info("Reconnecting system failed to connect");
+ listener.setShutdownCause(shutdownCause);
+ uncleanShutdown(reason,
+ new MemberDisconnectedException("reconnecting system failed to
connect"));
+ }
+
+ void uncleanShutdownDS(String reason, Exception shutdownCause) {
+ try {
+ listener.saveConfig();
+ } finally {
+ new LoggingThread("DisconnectThread", false, () -> {
+ lifecycleListener.forcedDisconnect();
+ listener.setShutdownCause(shutdownCause);
+ uncleanShutdown(reason, shutdownCause);
Review comment:
Why is the call to `uncleanShutdown()` done in the logging thread? Seems
to me that it should be done in the calling thread. Doing it in the calling
thread generally means it'll happen sooner and also it's easier to test.
If that's right it could be done at the end of the `finally` or the end of
the 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
