gesterzhou commented on a change in pull request #6827:
URL: https://github.com/apache/geode/pull/6827#discussion_r717054115
##########
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:
Here only refactored the existing code into a method, without changing
any logic.
See the replaced code, it changed cause to be ForcedDisconnectException.
##########
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:
I can add javadoc for this method.
Maybe the method name can be changed to
setPossibleShutdownCauseToForceDisconnectException(shutdownCause)
The method signature is from the refactoring above. Since the original code
expects to convert throwable into ForceDisconnectionException, thus this
setShutdownCause() should return a Throwable.
From listener point of view, we don't need to return anything. We did this
way is only to avoid duplicated code.
Of cause, I can complete revoked the code changes in
ClusterDistributionManager.java, then I can change to void
setShutdownCause(Throwable t) here, because in GMSMembership.java, we don't
need setShutdownCause() to return anything.
However, in our code, there will be 2 places with very similar code, one in
ClusterDistributionManager, one in GMSMembership. That's what we want to
avoid.
Conceptually, the whole fix's idea is to move setShutdownCause() a little
earlier in force disconnect case. So it makes sense to do the refactoring in
ClusterDistributionManager.DMListener into a method, then let listener to make
use of this method.
##########
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:
We did not change to use logging thread here. The original code called
uncleanShutdown() in logging thread.
Our fix here is only to add listener.setShutdownCause(shutdownCause) in
both isReconnectingDS() and not reconnecting case.
##########
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:
services.setShutdownCause(shutdownCause);
is different with listener.setShutdownCause(shutdownCause).
They are 2 separate steps in previous code.
services.setShutdownCause() set shutdownCause into Services class only,
while
ClusterDistributionManager.DMListener's setShutdownCause(shutdownCause) will
convert other cause into ForceDisconnectException and set into DM.
We call listener.setShutdownCause() in both if and else is because
listener.setShutdownCause() has to be called after listener.saveConfig() and
lifecycleListener.forcedDisconnect().
It cannot be called around line 1808.
To make thing clearer, maybe we can the listener's method name to be:
setPossibleShutdownCauseToForceDisconnectException(shutdownCause). Then it
will not mixed with services.setShutdownCause().
--
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]
