-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-20553 Add await self in local logical topology logic to node startup procedure #3220
Conversation
…-20553 # Conflicts: # modules/placement-driver/src/integrationTest/java/org/apache/ignite/internal/placementdriver/ActiveActorTest.java
…edByAnotherNodesAlready
/** | ||
* Retrieves the current logical topology snapshot stored in the local storage. | ||
*/ | ||
LogicalTopologySnapshot getLogicalTopology(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agee. I've used the mehod naming from internals, but you proposal looks reasonable.
LogicalTopologyEventListener awaitSelfListener = new LogicalTopologyEventListener() { | ||
@Override | ||
public void onNodeJoined(LogicalNode joinedNode, LogicalTopologySnapshot newTopology) { | ||
if (newTopology.nodes().stream().map(LogicalNode::id).collect(Collectors.toSet()).contains(id())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If toSet
is imported statically, this will read nicely in plain English: 'collect to set'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
@@ -990,6 +994,37 @@ public CompletableFuture<Ignite> start(Path configPath) { | |||
|
|||
return cmgMgr.onJoinReady(); | |||
}, startupExecutor) | |||
.thenComposeAsync(ignored -> { | |||
CompletableFuture<Void> awaitSelfInLogicalTopologyFuture = new CompletableFuture<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to extract this as a method? The method in which this code is defined is already huge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, make sense.
|
||
@Override | ||
public void onTopologyLeap(LogicalTopologySnapshot newTopology) { | ||
if (newTopology.nodes().stream().map(LogicalNode::id).collect(Collectors.toSet()).contains(id())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there are 3 duplicates of this code, it it possible to extract them to methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think that it's really needed, taking into considiration that everething inside thenCompose will be extracted I'd rather left inner duplication as is. However if you will intend I'll also add a private helper for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will add inner method.
https://issues.apache.org/jira/browse/IGNITE-20553