[
https://issues.apache.org/jira/browse/GEODE-8385?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17167013#comment-17167013
]
ASF GitHub Bot commented on GEODE-8385:
---------------------------------------
Bill commented on a change in pull request #5403:
URL: https://github.com/apache/geode/pull/5403#discussion_r461217750
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1866,41 +1862,54 @@ public void
handleConsoleShutdown(InternalDistributedMember theId, boolean crash
void shutdownMessageReceived(InternalDistributedMember theId, String reason)
{
removeHostedLocators(theId);
distribution.shutdownMessageReceived(theId, reason);
+ handleManagerDeparture(theId, false, reason, false);
}
@Override
- public void handleManagerDeparture(InternalDistributedMember theId, boolean
p_crashed,
- String p_reason) {
-
+ public void handleManagerDeparture(InternalDistributedMember theId, boolean
memberCrashed,
+ String reason, boolean fromViewChange) {
alertingService.removeAlertListener(theId);
+ removeUnfinishedStartup(theId, true);
+
int vmType = theId.getVmKind();
if (vmType == ADMIN_ONLY_DM_TYPE) {
- removeUnfinishedStartup(theId, true);
- handleConsoleShutdown(theId, p_crashed, p_reason);
+ handleConsoleShutdown(theId, memberCrashed, reason);
return;
}
- removeUnfinishedStartup(theId, true);
-
- if (removeManager(theId, p_crashed, p_reason)) {
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
+ if (!fromViewChange) {
+ if (!isCurrentMember(theId)) {
+ // this is notification from a shutdown message received from a member
that is no longer
+ // part of the cluster
+ return;
}
- String msg;
- if (p_crashed && !shouldInhibitMembershipWarnings()) {
- msg =
- "Member at {} unexpectedly left the distributed cache: {}";
- addMemberEvent(new MemberCrashedEvent(theId, p_reason));
- } else {
- msg =
- "Member at {} gracefully left the distributed cache: {}";
- addMemberEvent(new MemberDepartedEvent(theId, p_reason));
+ // else this is from a shutdown message so continue & notify listeners
+ } else {
+ if (!memberCrashed) {
+ // member left the view normally - we've already received a shutdown
message and notified
+ // listeners, so there's nothing more to do here
+ return;
}
- logger.info(msg, new Object[] {theId, prettifyReason(p_reason)});
+ }
- executors.handleManagerDeparture(theId);
+ removeManager(theId, memberCrashed, reason);
+ if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
+ stats.incNodes(-1);
+ }
+ String msg;
+ if (memberCrashed && !shouldInhibitMembershipWarnings()) {
+ msg =
+ "Member at {} unexpectedly left the distributed cache: {}";
+ addMemberEvent(new MemberCrashedEvent(theId, reason));
+ } else {
+ msg =
+ "Member at {} gracefully left the distributed cache: {}";
+ addMemberEvent(new MemberDepartedEvent(theId, reason));
}
+ logger.info(msg, new Object[] {theId, prettifyReason(reason)});
+
+ executors.handleManagerDeparture(theId);
Review comment:
✓ ok to this block of code—I confirmed it's the same as the old block
that was under the conditional but no conditional is needed now that
`removeManager()` returns `void` (and it used to always return `true`) ✓
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -2366,10 +2375,10 @@ public void memberDeparted(InternalDistributedMember
theId, boolean crashed, Str
message.setReason(reason); // added for #37950
handleIncomingDMsg(message);
}
- dm.handleManagerDeparture(theId, crashed, reason);
} catch (DistributedSystemDisconnectedException se) {
// let's not get huffy about it
}
+ dm.handleManagerDeparture(theId, crashed, reason, true);
Review comment:
why did this statement move? should it be in a `finally` block?
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
##########
@@ -399,11 +399,11 @@ public int hashCode() {
public String toString() {
StringBuilder sb = new StringBuilder(100);
- sb.append("MemberData[");
+ sb.append("GMSMember[");
Review comment:
Why did the name change to `"GMSMember"` here? It was `"MemberData"`
which kinda made sense because `GMSMemberData` isa `MemberData`. I could see
leaving it alone or maybe making it `GMSMemberData`.
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1784,19 +1784,15 @@ private String prettifyReason(String r) {
/**
* Returns true if id was removed. Returns false if it was not in the list
of managers.
*/
- private boolean removeManager(InternalDistributedMember theId, boolean
crashed, String p_reason) {
- String reason = p_reason;
-
- reason = prettifyReason(reason);
+ private void removeManager(InternalDistributedMember theId, boolean crashed,
String p_reason) {
Review comment:
comment lies now that this returns `void`
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1866,41 +1848,45 @@ public void
handleConsoleShutdown(InternalDistributedMember theId, boolean crash
void shutdownMessageReceived(InternalDistributedMember theId, String reason)
{
removeHostedLocators(theId);
distribution.shutdownMessageReceived(theId, reason);
+ handleManagerDeparture(theId, false, reason, false);
}
@Override
- public void handleManagerDeparture(InternalDistributedMember theId, boolean
p_crashed,
- String p_reason) {
-
+ public void handleManagerDeparture(InternalDistributedMember theId, boolean
memberCrashed,
+ String reason, boolean fromViewChange) {
alertingService.removeAlertListener(theId);
+ removeUnfinishedStartup(theId, true);
Review comment:
nice
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1866,41 +1848,45 @@ public void
handleConsoleShutdown(InternalDistributedMember theId, boolean crash
void shutdownMessageReceived(InternalDistributedMember theId, String reason)
{
removeHostedLocators(theId);
distribution.shutdownMessageReceived(theId, reason);
+ handleManagerDeparture(theId, false, reason, false);
}
@Override
- public void handleManagerDeparture(InternalDistributedMember theId, boolean
p_crashed,
- String p_reason) {
-
+ public void handleManagerDeparture(InternalDistributedMember theId, boolean
memberCrashed,
+ String reason, boolean fromViewChange) {
alertingService.removeAlertListener(theId);
+ removeUnfinishedStartup(theId, true);
+
int vmType = theId.getVmKind();
if (vmType == ADMIN_ONLY_DM_TYPE) {
- removeUnfinishedStartup(theId, true);
- handleConsoleShutdown(theId, p_crashed, p_reason);
+ handleConsoleShutdown(theId, memberCrashed, reason);
return;
}
- removeUnfinishedStartup(theId, true);
-
- if (removeManager(theId, p_crashed, p_reason)) {
- if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
- stats.incNodes(-1);
- }
- String msg;
- if (p_crashed && !shouldInhibitMembershipWarnings()) {
- msg =
- "Member at {} unexpectedly left the distributed cache: {}";
- addMemberEvent(new MemberCrashedEvent(theId, p_reason));
- } else {
- msg =
- "Member at {} gracefully left the distributed cache: {}";
- addMemberEvent(new MemberDepartedEvent(theId, p_reason));
- }
- logger.info(msg, new Object[] {theId, prettifyReason(p_reason)});
+ if (logger.isDebugEnabled()) {
+ logger.debug("DistributionManager: removing member <{}>; crashed {};
reason = {}", theId,
+ memberCrashed, prettifyReason(reason));
+ }
+ removeHostedLocators(theId);
+ redundancyZones.remove(theId);
Review comment:
✓ inlined `removeManager()`
----------------------------------------------------------------
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]
> hang recovering from disk with cyclic dependencies
> --------------------------------------------------
>
> Key: GEODE-8385
> URL: https://issues.apache.org/jira/browse/GEODE-8385
> Project: Geode
> Issue Type: Bug
> Components: membership, persistence
> Reporter: Bruce J Schuchardt
> Assignee: Bruce J Schuchardt
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.13.0
>
>
> In a test cluster using replicated persistent Regions all of the servers were
> shut down and restarted. The restart hung showing a cycle in disk store
> dependencies.
> {noformat}
> [info 2020/05/29 03:02:36.635 PDT <Thread-18> tid=0x8f] Region /Region_14 has
> potentially stale data. It is waiting for another online member to recover
> the latest data.My persistent id:
> DiskStore ID: a175354a-d27d-4575-9916-16fd7ff7ea67 Name:
> persistgemfire4_host1_4194 Location:
> /10.32.110.100:/var/vcap/data/rundir/concRecoverAllV4O41/concRecoverAll-0529-024642/vm_5_persist4_disk_1
> Members with potentially new data:[
> DiskStore ID: 2d77752e-507d-4425-a382-a5856c61938f Name:
> persistgemfire10_host1_4208 Location:
> /10.32.110.100:/var/vcap/data/rundir/concRecoverAllV4O41/concRecoverAll-0529-024642/vm_2_persist10_disk_1]
> Use the gfsh show missing-disk-stores command to see all disk stores that are
> being waited on by other members.
> {noformat}
> After looking at the logs for all members, the "members with potentially new
> data" for each member were found to be:
> {noformat}
> Member | Members with potentially new data
> --------+----------------------------------
> 1 | all
> 2 | 4
> 3 | 4
> 4 | 10
> 5 | 2, 3, 4, 8, 10
> 6 | 2, 3, 4, 5, 7, 8, 10
> 7 | 3, 4, 10
> 8 | 3, 4, 10
> 9 | 2, 3, 4, 5, 7, 8, 10
> 10 | 3
> {noformat}
> It appears that there is a cycle in this "waiting for another online member"
> graph between 3 > 4 > 10 > 3.
> The problem seems to have cropped up after the fix for GEODE-7196 was merged.
> That changed the timing of member-departed notifications such that a server
> might close a Region's Persistence Advisor before getting notification that
> another server was shutting down. We used to do this notification upon
> receipt of a ShutdownMessage but now we only do it when the membership view
> has changed.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
