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/05/13 11:25:59 UTC
[GitHub] [ignite-3] ascherbakoff opened a new pull request #126: IGNITE-14707
ascherbakoff opened a new pull request #126:
URL: https://github.com/apache/ignite-3/pull/126
--
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] [ignite-3] ascherbakoff commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631819435
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
+ this.members.put(member.address(), member);
- break;
-
- case REMOVED:
- // In case if member left non-gracefully, without sending LEAVING event.
- if (members.remove(memberName) != null)
- fireDisappearedEvent(member);
-
- break;
-
- case UPDATED:
- // No-op.
- break;
-
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ fireAppearedEvent(member);
+ }
+ else if (event.isRemoved()) {
+ this.members.compute(member.address(), // Ignore stale remove event.
+ (k, v) -> v.id().equals(member.id()) ? null : v);
+ fireDisappearedEvent(member);
Review comment:
The event is "stale" only in the context of membership view.
But, because each event now is bound to specific cluster id, it has to be fired or it will be lost.
For example, it's a correct event reordering on some cluster node:
join A(id=1)
fail A(id=1)
join A(id=2)
--->
join A(id=1)
join A(id=2)
fail(A(id=1)
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
+ this.members.put(member.address(), member);
- break;
-
- case REMOVED:
- // In case if member left non-gracefully, without sending LEAVING event.
- if (members.remove(memberName) != null)
- fireDisappearedEvent(member);
-
- break;
-
- case UPDATED:
- // No-op.
- break;
-
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ fireAppearedEvent(member);
+ }
+ else if (event.isRemoved()) {
+ this.members.compute(member.address(), // Ignore stale remove event.
+ (k, v) -> v.id().equals(member.id()) ? null : v);
+ fireDisappearedEvent(member);
Review comment:
The event is "stale" only in the context of membership view.
But, because each event now is bound to specific cluster id, it has to be fired or it will be lost.
For example, it's a correct event reordering on some cluster node B:
join A(id=1)
fail A(id=1)
join A(id=2)
--->
join A(id=1)
join A(id=2)
fail A(id=1)
--
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] [ignite-3] ascherbakoff closed pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff closed pull request #126:
URL: https://github.com/apache/ignite-3/pull/126
--
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] [ignite-3] SammyVimes commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631822917
##########
File path: modules/network/src/test/resources/simplelogger.properties
##########
@@ -0,0 +1,52 @@
+#
Review comment:
Ok
--
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] [ignite-3] SammyVimes commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631765180
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -36,13 +34,13 @@
private ClusterNode localMember;
/** Topology members. */
- private final Map<String, ClusterNode> members = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String, ClusterNode> members = new ConcurrentHashMap<>();
Review comment:
```suggestion
private final ConcurrentMap<String, ClusterNode> members = new ConcurrentHashMap<>();
```
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -36,13 +34,13 @@
private ClusterNode localMember;
/** Topology members. */
- private final Map<String, ClusterNode> members = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String, ClusterNode> members = new ConcurrentHashMap<>();
/**
* Sets the ScaleCube's local {@link Member}.
*/
void setLocalMember(Member member) {
- localMember = fromMember(member);
+ this.localMember = fromMember(member);
Review comment:
isn't `this` odd here?
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
Review comment:
what about `LEAVING` event?
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
+ this.members.put(member.address(), member);
- break;
-
- case REMOVED:
- // In case if member left non-gracefully, without sending LEAVING event.
- if (members.remove(memberName) != null)
- fireDisappearedEvent(member);
-
- break;
-
- case UPDATED:
- // No-op.
- break;
-
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ fireAppearedEvent(member);
+ }
+ else if (event.isRemoved()) {
+ this.members.compute(member.address(), // Ignore stale remove event.
+ (k, v) -> v.id().equals(member.id()) ? null : v);
+ fireDisappearedEvent(member);
Review comment:
I guess this should not be called if the event is stale
--
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] [ignite-3] ascherbakoff commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631820378
##########
File path: modules/network/src/test/resources/simplelogger.properties
##########
@@ -0,0 +1,52 @@
+#
Review comment:
Don't see any problems here. It's ok to have logs for analysis in tests.
--
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] [ignite-3] SammyVimes commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631785579
##########
File path: modules/network/src/integrationTest/java/org/apache/ignite/network/scalecube/ITNodeRestartsTest.java
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ignite.network.scalecube;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.IntStream;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.network.ClusterLocalConfiguration;
+import org.apache.ignite.network.ClusterService;
+import org.apache.ignite.network.ClusterServiceFactory;
+import org.apache.ignite.network.message.MessageSerializationRegistry;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import static java.util.stream.Collectors.toList;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests if a topology size is correct after some nodes are restarted in quick succession.
+ */
+class ITNodeRestartsTest {
+ /** */
+ private static final IgniteLogger LOG = IgniteLogger.forClass(ITNodeRestartsTest.class);
+
+ /** */
+ private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry();
Review comment:
Let's make this and NETWORK_FACTORY non-static
--
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] [ignite-3] ascherbakoff commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631821892
##########
File path: modules/network/src/integrationTest/java/org/apache/ignite/network/scalecube/ITNodeRestartsTest.java
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ignite.network.scalecube;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.IntStream;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.network.ClusterLocalConfiguration;
+import org.apache.ignite.network.ClusterService;
+import org.apache.ignite.network.ClusterServiceFactory;
+import org.apache.ignite.network.message.MessageSerializationRegistry;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import static java.util.stream.Collectors.toList;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests if a topology size is correct after some nodes are restarted in quick succession.
+ */
+class ITNodeRestartsTest {
+ /** */
+ private static final IgniteLogger LOG = IgniteLogger.forClass(ITNodeRestartsTest.class);
+
+ /** */
+ private static final MessageSerializationRegistry SERIALIZATION_REGISTRY = new MessageSerializationRegistry();
Review comment:
Why ?
SERIALIZATION_REGISTRY and NETWORK_FACTORY are similarly static in org.apache.ignite.network.scalecube.ITScaleCubeNetworkMessagingTest.Cluster
and logger has to be static final.
--
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] [ignite-3] ascherbakoff commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631819435
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
+ this.members.put(member.address(), member);
- break;
-
- case REMOVED:
- // In case if member left non-gracefully, without sending LEAVING event.
- if (members.remove(memberName) != null)
- fireDisappearedEvent(member);
-
- break;
-
- case UPDATED:
- // No-op.
- break;
-
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ fireAppearedEvent(member);
+ }
+ else if (event.isRemoved()) {
+ this.members.compute(member.address(), // Ignore stale remove event.
+ (k, v) -> v.id().equals(member.id()) ? null : v);
+ fireDisappearedEvent(member);
Review comment:
The event is "stale" only in the context of membership view.
But, because each event now is bound to specific cluster id, it has to be fired or it will be lost.
For example, it's a correct event reordering:
join A(id=1) join A(id=1)
fail A(id=1) ---> join A(id=2)
join A(id=2) fail(A(id=1)
--
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] [ignite-3] SammyVimes commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631823043
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
+ this.members.put(member.address(), member);
- break;
-
- case REMOVED:
- // In case if member left non-gracefully, without sending LEAVING event.
- if (members.remove(memberName) != null)
- fireDisappearedEvent(member);
-
- break;
-
- case UPDATED:
- // No-op.
- break;
-
- default:
- throw new IgniteInternalException("This event is not supported: event = " + event);
+ fireAppearedEvent(member);
+ }
+ else if (event.isRemoved()) {
+ this.members.compute(member.address(), // Ignore stale remove event.
+ (k, v) -> v.id().equals(member.id()) ? null : v);
+ fireDisappearedEvent(member);
Review comment:
Thanks for explanation!
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
Review comment:
Ok, why not
--
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] [ignite-3] SammyVimes commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631784591
##########
File path: modules/network/src/test/resources/simplelogger.properties
##########
@@ -0,0 +1,52 @@
+#
Review comment:
This configuration leads to excessive amount of logs (courtesy of scalecube)
--
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] [ignite-3] ascherbakoff commented on a change in pull request #126: IGNITE-14707
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #126:
URL: https://github.com/apache/ignite-3/pull/126#discussion_r631816346
##########
File path: modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java
##########
@@ -54,37 +52,16 @@ void setLocalMember(Member member) {
void onMembershipEvent(MembershipEvent event) {
ClusterNode member = fromMember(event.member());
- String memberName = member.name();
-
- switch (event.type()) {
- case ADDED:
- members.put(memberName, member);
-
- fireAppearedEvent(member);
-
- break;
-
- case LEAVING:
- members.remove(memberName);
-
- fireDisappearedEvent(member);
+ if (event.isAdded()) {
Review comment:
LEAVING event makes sense only if we want to let the user know the node has left intentionally - similar to NODE_LEFT event in ignite2.
Currently I see no demand in such functionality. It may be added later, if needed.
--
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