sashapolo commented on code in PR #7191:
URL: https://github.com/apache/ignite-3/pull/7191#discussion_r2602567376
##########
modules/network/src/test/java/org/apache/ignite/internal/network/recovery/RecoveryAcceptorHandshakeManagerTest.java:
##########
@@ -102,6 +103,9 @@ class RecoveryAcceptorHandshakeManagerTest extends
HandshakeManagerTest {
@Captor
private ArgumentCaptor<OutNetworkObject> sentMessageCaptor;
+ @Mock
+ protected TopologyService topologyService;
Review Comment:
I'm clearly missing something here, I'll stop asking the same question)
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/ConnectionManager.java:
##########
@@ -525,41 +494,50 @@ public boolean isStopped() {
private HandshakeManager createInitiatorHandshakeManager(short
connectionId) {
InternalClusterNode localNode =
Objects.requireNonNull(localNodeFuture.getNow(null), "localNode not set");
- if (initiatorHandshakeManagerFactory == null) {
- return new RecoveryInitiatorHandshakeManager(
- localNode,
- connectionId,
- descriptorProvider,
- bootstrapFactory.handshakeEventLoopSwitcher(),
- staleIdDetector,
- clusterIdSupplier,
- this,
- stopping::get,
- productVersionSource
- );
- }
+ return newRecoveryInitiatorHandshakeManager(connectionId, localNode);
+ }
- return initiatorHandshakeManagerFactory.create(
+ /**
+ * Factory method for overriding the handshake manager implementation in
subclasses.
+ */
+ protected RecoveryInitiatorHandshakeManager
newRecoveryInitiatorHandshakeManager(
+ short connectionId,
+ InternalClusterNode localNode
+ ) {
+ return new RecoveryInitiatorHandshakeManager(
localNode,
connectionId,
- descriptorProvider
+ descriptorProvider,
+ bootstrapFactory.handshakeEventLoopSwitcher(),
+ staleIdDetector,
+ clusterIdSupplier,
+ this,
+ stopping::get,
+ productVersionSource,
+ topologyService,
+ failureProcessor
);
}
private HandshakeManager createAcceptorHandshakeManager() {
// Do not just use localNodeFuture.join() to make sure the wait is
time-limited.
waitForLocalNodeToBeSet();
+ return newRecoveryAcceptorHandshakeManager(localNodeFuture.join());
Review Comment:
Not related to this PR, but this is super weird that we first call
`waitForLocalNodeToBeSet` (which calls `CompletableFuture#get`, but doesn't
return the result) only to then call `localNodeFuture.join()`
##########
modules/network/src/integrationTest/java/org/apache/ignite/internal/network/netty/ItConnectionManagerTest.java:
##########
@@ -564,7 +567,9 @@ private ConnectionManagerWrapper startManager(int port,
UUID launchId, MessageSe
new AllIdsAreFresh(),
withoutClusterId(),
defaultChannelTypeRegistry(),
- new DefaultIgniteProductVersionSource()
+ new DefaultIgniteProductVersionSource(),
+ mock(TopologyService.class),
+ new FailureManager(new NoOpFailureHandler())
Review Comment:
Why not use a `NoOpFailureManager`?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/recovery/RecoveryAcceptorHandshakeManager.java:
##########
@@ -105,6 +106,9 @@ public class RecoveryAcceptorHandshakeManager implements
HandshakeManager {
/** Recovery descriptor. */
private RecoveryDescriptor recoveryDescriptor;
+ /** Cluster topology service. */
+ protected final TopologyService topologyService;
Review Comment:
Why is this field `protected`?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/recovery/RecoveryAcceptorHandshakeManager.java:
##########
@@ -123,7 +128,7 @@ public RecoveryAcceptorHandshakeManager(
ClusterIdSupplier clusterIdSupplier,
ChannelCreationListener channelCreationListener,
BooleanSupplier stopping,
- IgniteProductVersionSource productVersionSource
+ IgniteProductVersionSource productVersionSource, TopologyService
topologyService
Review Comment:
This formatting looks bad
##########
modules/network/src/main/java/org/apache/ignite/internal/network/recovery/RecoveryInitiatorHandshakeManager.java:
##########
@@ -114,6 +116,12 @@ public class RecoveryInitiatorHandshakeManager implements
HandshakeManager {
/** Recovery descriptor. */
private RecoveryDescriptor recoveryDescriptor;
+ /** Cluster topology service. */
+ protected final TopologyService topologyService;
Review Comment:
And why are these fields `protected`?
--
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]
