You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2024/02/23 13:36:13 UTC
Re: [PR] IGNITE-20553 Add await self in local logical topology logic to node startup procedure [ignite-3]
rpuch commented on code in PR #3220:
URL: https://github.com/apache/ignite-3/pull/3220#discussion_r1500669106
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalTopologyService.java:
##########
@@ -62,6 +62,11 @@ public interface LogicalTopologyService {
*/
CompletableFuture<LogicalTopologySnapshot> logicalTopologyOnLeader();
+ /**
+ * Retrieves the current logical topology snapshot stored in the local storage.
+ */
+ LogicalTopologySnapshot getLogicalTopology();
Review Comment:
Does it make sense to reflect it in the name of the method that the topology is returned based on the local view of the node? Like `localLogicalTopology()`.
Also, `get` prefix does not match the style of `logicalTopologyOnLeader()`: they should both have `get` prefix, or none of them should have it.
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -990,6 +994,37 @@ public CompletableFuture<Ignite> start(Path configPath) {
return cmgMgr.onJoinReady();
}, startupExecutor)
+ .thenComposeAsync(ignored -> {
+ CompletableFuture<Void> awaitSelfInLogicalTopologyFuture = new CompletableFuture<>();
+
+ LogicalTopologyEventListener awaitSelfListener = new LogicalTopologyEventListener() {
+ @Override
+ public void onNodeJoined(LogicalNode joinedNode, LogicalTopologySnapshot newTopology) {
+ if (newTopology.nodes().stream().map(LogicalNode::id).collect(Collectors.toSet()).contains(id())) {
Review Comment:
If `toSet` is imported statically, this will read nicely in plain English: 'collect to set'
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -990,6 +994,37 @@ public CompletableFuture<Ignite> start(Path configPath) {
return cmgMgr.onJoinReady();
}, startupExecutor)
+ .thenComposeAsync(ignored -> {
+ CompletableFuture<Void> awaitSelfInLogicalTopologyFuture = new CompletableFuture<>();
+
+ LogicalTopologyEventListener awaitSelfListener = new LogicalTopologyEventListener() {
+ @Override
+ public void onNodeJoined(LogicalNode joinedNode, LogicalTopologySnapshot newTopology) {
+ if (newTopology.nodes().stream().map(LogicalNode::id).collect(Collectors.toSet()).contains(id())) {
+ awaitSelfInLogicalTopologyFuture.complete(null);
+ logicalTopologyService.removeEventListener(this);
+ }
+ }
+
+ @Override
+ public void onTopologyLeap(LogicalTopologySnapshot newTopology) {
+ if (newTopology.nodes().stream().map(LogicalNode::id).collect(Collectors.toSet()).contains(id())) {
Review Comment:
It seems like there are 3 duplicates of this code, it it possible to extract them to methods?
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -990,6 +994,37 @@ public CompletableFuture<Ignite> start(Path configPath) {
return cmgMgr.onJoinReady();
}, startupExecutor)
+ .thenComposeAsync(ignored -> {
+ CompletableFuture<Void> awaitSelfInLogicalTopologyFuture = new CompletableFuture<>();
Review Comment:
Is it possible to extract this as a method? The method in which this code is defined is already huge.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org