You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/04/14 15:26:23 UTC
[GitHub] [ignite-3] sashapolo commented on a change in pull request #94: IGNITE-14536 Handle forceful shutdown of ScaleCube cluster node.
sashapolo commented on a change in pull request #94:
URL: https://github.com/apache/ignite-3/pull/94#discussion_r613343476
##########
File path: modules/network/src/integrationTest/java/org/apache/ignite/network/scalecube/ITScaleCubeNetworkMessagingTest.java
##########
@@ -144,4 +196,68 @@ private ClusterService startNetwork(String name, int port, List<String> addresse
return clusterService;
}
+
+ /**
+ * Find cluster's transport and force it to stop.
+ * @param cluster Cluster to be shutdown.
+ * @throws Exception If failed to stop.
+ */
+ private static void stopForcefully(ClusterService cluster) throws Exception {
+ Field clusterImplField = cluster.getClass().getDeclaredField("val$cluster");
+ clusterImplField.setAccessible(true);
+
+ ClusterImpl clusterImpl = (ClusterImpl) clusterImplField.get(cluster);
+ Field transportField = clusterImpl.getClass().getDeclaredField("transport");
+ transportField.setAccessible(true);
+
+ Transport transport = (Transport) transportField.get(clusterImpl);
+ Method stop = transport.getClass().getDeclaredMethod("stop");
+ stop.setAccessible(true);
+
+ Mono invoke = (Mono) stop.invoke(transport);
Review comment:
```suggestion
Mono<?> invoke = (Mono<?>) stop.invoke(transport);
```
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -52,31 +52,50 @@ void setLocalMember(Member member) {
*/
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- for (TopologyEventHandler handler : getEventHandlers()) {
- switch (event.type()) {
- case ADDED:
- members.put(member.name(), member);
- handler.onAppeared(member);
+ String memberName = member.name();
+
+ boolean appeared = false;
+ boolean disappeared = false;
+
+ switch (event.type()) {
+ case ADDED:
+ members.put(memberName, member);
+
+ appeared = true;
+
+ break;
- break;
+ case LEAVING:
+ members.remove(memberName);
- case LEAVING:
- members.remove(member.name());
+ disappeared = true;
- handler.onDisappeared(member);
+ break;
- break;
+ case REMOVED:
+ if (members.containsKey(memberName)) {
Review comment:
Looks like this check is unnecessary:
```
if (members.remove(memberName) != null)
disappeared = true;
```
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -52,31 +52,50 @@ void setLocalMember(Member member) {
*/
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- for (TopologyEventHandler handler : getEventHandlers()) {
- switch (event.type()) {
- case ADDED:
- members.put(member.name(), member);
- handler.onAppeared(member);
+ String memberName = member.name();
+
+ boolean appeared = false;
+ boolean disappeared = false;
+
+ switch (event.type()) {
+ case ADDED:
+ members.put(memberName, member);
+
+ appeared = true;
+
+ break;
- break;
+ case LEAVING:
+ members.remove(memberName);
- case LEAVING:
- members.remove(member.name());
+ disappeared = true;
- handler.onDisappeared(member);
+ break;
- break;
+ case REMOVED:
+ if (members.containsKey(memberName)) {
+ members.remove(memberName);
- case REMOVED:
- case UPDATED:
- // No-op.
- break;
+ disappeared = true;
+ }
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ break;
- }
+ case UPDATED:
+ // No-op.
+ break;
+
+ default:
+ throw new IgniteInternalException("This event is not supported: event = " + event);
+
+ }
+
+ for (TopologyEventHandler handler : getEventHandlers()) {
Review comment:
We will re-iterate over all event handlers even if we've received a non-interesting message (e.g. `UPDATED`). I think we can optimize this approach. For example, I would suggest using two methods (`fireAppearedEvent` and `fireDissapearedEvent`) and get rid of these two flags
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -52,31 +52,50 @@ void setLocalMember(Member member) {
*/
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- for (TopologyEventHandler handler : getEventHandlers()) {
- switch (event.type()) {
- case ADDED:
- members.put(member.name(), member);
- handler.onAppeared(member);
+ String memberName = member.name();
+
+ boolean appeared = false;
+ boolean disappeared = false;
+
+ switch (event.type()) {
+ case ADDED:
+ members.put(memberName, member);
+
+ appeared = true;
+
+ break;
- break;
+ case LEAVING:
+ members.remove(memberName);
- case LEAVING:
- members.remove(member.name());
+ disappeared = true;
- handler.onDisappeared(member);
+ break;
- break;
+ case REMOVED:
+ if (members.containsKey(memberName)) {
Review comment:
I would also suggest leaving a comment about what case are we handling here
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java
##########
@@ -51,7 +52,7 @@
topologyService.onMembershipEvent(event);
}
})
- .config(opts -> opts.memberAlias(context.getName()))
+ .config(opts -> opts.memberAlias(context.getName()).membership(config -> config.suspicionMult(1)))
Review comment:
please add a comment that this timeout value is currently needed for tests
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java
##########
@@ -51,7 +52,7 @@
topologyService.onMembershipEvent(event);
}
})
- .config(opts -> opts.memberAlias(context.getName()))
+ .config(opts -> opts.memberAlias(context.getName()).membership(config -> config.suspicionMult(1)))
Review comment:
also, please move the multiplier configuration to the `membership` configuration on line 57
--
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