rpuch commented on code in PR #5345:
URL: https://github.com/apache/ignite-3/pull/5345#discussion_r1984868108
##########
modules/network-api/src/main/java/org/apache/ignite/internal/network/NodeFinder.java:
##########
@@ -36,4 +36,7 @@ public interface NodeFinder extends ManuallyCloseable {
default void close() {
// No-op.
}
+
+ /** Start the node finder. */
Review Comment:
```suggestion
/** Starts the node finder. */
```
##########
modules/network/src/integrationTest/java/org/apache/ignite/internal/network/scalecube/ItMulticastNodeFinderTest.java:
##########
@@ -67,34 +70,46 @@ void setUp(TestInfo testInfo) {
@AfterEach
void tearDown() {
assertThat(stopAsync(new ComponentContext(), services),
willCompleteSuccessfully());
+ services.clear();
+ finders.forEach(NodeFinder::close);
Review Comment:
Let's use `IgniteUtils.closeAll()` to close as many 'things' we can, even if
some of them throw an exception.
##########
modules/network/src/main/java/org/apache/ignite/internal/network/MulticastNodeFinder.java:
##########
@@ -85,21 +91,23 @@ public class MulticastNodeFinder implements NodeFinder {
* @param multicastPort Multicast port.
* @param resultWaitMillis Wait time for responses.
* @param ttl Time-to-live for multicast packets.
- * @param localAddress Local node address.
+ * @param nodeName Node name.
+ * @param localAddressToAdvertise Local node address.
*/
public MulticastNodeFinder(
String multicastGroup,
int multicastPort,
int resultWaitMillis,
int ttl,
- InetSocketAddress localAddress
+ String nodeName,
+ InetSocketAddress localAddressToAdvertise
) {
this.multicastSocketAddress = new InetSocketAddress(multicastGroup,
multicastPort);
this.multicastPort = multicastPort;
this.resultWaitMillis = resultWaitMillis;
this.ttl = ttl;
- this.localAddress = localAddress;
- this.threadPool = Executors.newFixedThreadPool(4);
+ this.localAddressToAdvertise = localAddressToAdvertise;
+ this.threadPool = Executors.newFixedThreadPool(4,
NamedThreadFactory.create(nodeName, "rocksdb-storage-engine-pool", LOG));
Review Comment:
Something is wrong with the pool name :)
Also, I wonder, how 4 was chosen? It seems to be too much to keep 4 threads
just to exchange with these tiny messages. Would it make sense to use *up to N*
threads and also choose a smaller N?
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -497,6 +497,9 @@ public static class Network {
/** Could not resolve address. */
public static final int ADDRESS_UNRESOLVED_ERR =
NETWORK_ERR_GROUP.registerErrorCode((short) 6);
+
+ /** Could not find nodes. */
+ public static final int NODE_FINDER_ERR =
NETWORK_ERR_GROUP.registerErrorCode((short) 7);
Review Comment:
Error codes are a part of the public API, only exceptions that can be caught
by users need to have such codes. But this code is used for exceptions that
might only be observed by our code. I think the error code should not be
introduced; instead, just use the INTERNAL_ERR code.
##########
modules/network/src/integrationTest/java/org/apache/ignite/internal/network/scalecube/ItMulticastNodeFinderTest.java:
##########
@@ -49,13 +50,15 @@
@ExtendWith(ExecutorServiceExtension.class)
class ItMulticastNodeFinderTest extends IgniteAbstractTest {
private static final int INIT_PORT = 3344;
-
+ private static final int MULTICAST_PORT = 3343;
Review Comment:
Please change it to look completely different (like 20000), otherwise I
would still be confused :)
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
