sashapolo commented on code in PR #1267:
URL: https://github.com/apache/ignite-3/pull/1267#discussion_r1019061936
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/Main.java:
##########
@@ -80,7 +80,7 @@ public static void main(String[] args) {
}
private static boolean isatty() {
- return System.console() != null;
+ return true;
Review Comment:
What is this change for?
##########
modules/network/src/integrationTest/java/org/apache/ignite/network/scalecube/ItClusterServiceTest.java:
##########
@@ -61,4 +68,59 @@ void testShutdown(TestInfo testInfo) {
assertThat(e.getCause(), isA(NodeStoppingException.class));
}
+
+ @Test
+ void testUpdateMetadata(TestInfo testInfo) throws Exception {
Review Comment:
Formatting here is a total mess, please add some empty lines
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java:
##########
@@ -241,4 +253,8 @@ private static List<Address>
parseAddresses(List<NetworkAddress> addresses) {
.map(addr -> Address.create(addr.host(), addr.port()))
.collect(Collectors.toList());
}
+
+ private Scheduler scheduler(String namePrefix) {
Review Comment:
Looks like this method is no longer needed
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java:
##########
@@ -104,6 +115,18 @@ void onMembershipEvent(MembershipEvent event) {
}
}
+ /**
+ * Updates info about the local member.
+ *
+ * @param member the local member.
+ * @param metadata metadata of the local member.
+ */
+ void updateLocalMember(Member member, @Nullable NodeMetadata metadata) {
Review Comment:
This is strange that we need to pass a `member` to the `updateLocalMember`
method. `TopologyService` has a link to the local member already
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeTopologyService.java:
##########
@@ -160,9 +184,25 @@ public ClusterNode getByConsistentId(String consistentId) {
* @param member ScaleCube's cluster member.
* @return Cluster node.
*/
- private static ClusterNode fromMember(Member member) {
+ private static ClusterNode fromMember(Member member, @Nullable
NodeMetadata nodeMetadata) {
var addr = new NetworkAddress(member.address().host(),
member.address().port());
- return new ClusterNode(member.id(), member.alias(), addr);
+ return new ClusterNode(member.id(), member.alias(), addr,
nodeMetadata);
+ }
+
+
+ /**
+ * Deserializes the given {@link ByteBuffer} to a {@link NodeMetadata}.
+ *
+ * @param buffer ByteBuffer to deserialize.
+ * @return NodeMetadata or null if something goes wrong.
+ */
+ private static NodeMetadata deserializeMetadata(ByteBuffer buffer) {
+ try {
+ return (NodeMetadata) METADATA_CODEC.deserialize(buffer);
Review Comment:
`buffer` can be null, this only works because you catch the NPE below. Let's
add an explicit check
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java:
##########
@@ -143,10 +151,8 @@ public void onMembershipEvent(MembershipEvent event) {
cluster.startAwait();
- // emit an artificial event as if the local member has joined
the topology (ScaleCube doesn't do that)
- var localMembershipEvent =
MembershipEvent.createAdded(cluster.member(), null, System.currentTimeMillis());
-
- topologyService.onMembershipEvent(localMembershipEvent);
+ // update info about the local member (ScaleCube doesn't emit
events about local members)
+ topologyService.updateLocalMember(cluster.member(), null);
Review Comment:
This is not a correct change. We rely on the fact that adding a local member
will trigger topology listeners (`onAppeared`), I wonder if the tests will even
pass
--
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]
