[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1295: IGNITE-17814 Use consistent IDs in Jraft instead of addresses

[GitBox <gi...@apache.org>]

ibessonov commented on code in PR #1295:
URL: https://github.com/apache/ignite-3/pull/1295#discussion_r1011545887


##########
modules/network/src/main/java/org/apache/ignite/network/DefaultMessagingService.java:
##########
@@ -214,31 +180,31 @@ private CompletableFuture<NetworkMessage> invoke0(@Nullable ClusterNode recipien
 
         requestsMap.put(correlationId, responseFuture);
 
-        InetSocketAddress address = new InetSocketAddress(addr.host(), addr.port());
+        InetSocketAddress recipientAddress = new InetSocketAddress(recipient.address().host(), recipient.address().port());

Review Comment:
   Same here



##########
modules/network/src/main/java/org/apache/ignite/network/DefaultMessagingService.java:
##########
@@ -128,54 +115,37 @@ public void weakSend(ClusterNode recipient, NetworkMessage msg) {
     /** {@inheritDoc} */
     @Override
     public CompletableFuture<Void> send(ClusterNode recipient, NetworkMessage msg) {
-        return send0(recipient, recipient.address(), msg, null);
+        return send0(recipient, msg, null);
     }
 
     /** {@inheritDoc} */
     @Override
     public CompletableFuture<Void> respond(ClusterNode recipient, NetworkMessage msg, long correlationId) {
-        return send0(recipient, recipient.address(), msg, correlationId);
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public CompletableFuture<Void> respond(NetworkAddress addr, NetworkMessage msg, long correlationId) {
-        ClusterNode recipient = topologyService.getByAddress(addr);
-        return send0(recipient, addr, msg, correlationId);
+        return send0(recipient, msg, correlationId);
     }
 
     /** {@inheritDoc} */
     @Override
     public CompletableFuture<NetworkMessage> invoke(ClusterNode recipient, NetworkMessage msg, long timeout) {
-        return invoke0(recipient, recipient.address(), msg, timeout);
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public CompletableFuture<NetworkMessage> invoke(NetworkAddress addr, NetworkMessage msg, long timeout) {
-        ClusterNode recipient = topologyService.getByAddress(addr);
-
-        return invoke0(recipient, addr, msg, timeout);
+        return invoke0(recipient, msg, timeout);
     }
 
     /**
      * Sends a message. If the target is the current node, then message will be delivered immediately.
      *
-     * @param recipient Target cluster node. TODO: Maybe {@code null} due to IGNITE-16373.
-     * @param address Target address.
+     * @param recipient Target cluster node.
      * @param msg Message.
      * @param correlationId Correlation id. Not null iff the message is a response to a {@link #invoke} request.
      * @return Future of the send operation.
      */
-    private CompletableFuture<Void> send0(@Nullable ClusterNode recipient, NetworkAddress address, NetworkMessage msg,
-            @Nullable Long correlationId) {
+    private CompletableFuture<Void> send0(ClusterNode recipient, NetworkMessage msg, @Nullable Long correlationId) {
         if (connectionManager.isStopped()) {
             return failedFuture(new NodeStoppingException());
         }
 
-        InetSocketAddress addr = new InetSocketAddress(address.host(), address.port());
+        InetSocketAddress recipientAddress = new InetSocketAddress(recipient.address().host(), recipient.address().port());

Review Comment:
   Will this be resolved every time? From the Ignite 2.x I know that it may take a while, so we should think about it before the release. Not right now



##########
modules/network/src/main/java/org/apache/ignite/network/DefaultMessagingService.java:
##########
@@ -375,33 +334,27 @@ private long createCorrelationId() {
     /**
      * Checks if the target is the current node.
      *
-     * @param target Target cluster node. TODO: IGNITE-16373 May be {@code null} due to the ticket.
-     * @param targetSocketAddress Target's socket address.
+     * @param consistentId Target consistent ID. Can be {@code null} if the node has not been added to the topology.
+     * @param targetAddress Target address.
      * @return {@code true} if the target is the current node, {@code false} otherwise.
      */
-    private boolean isSelf(@Nullable ClusterNode target, @Nullable String consistentId, SocketAddress targetSocketAddress) {
-        String cid = consistentId;
-
-        if (cid == null && target != null) {
-            cid = target.name();
+    private boolean isSelf(@Nullable String consistentId, InetSocketAddress targetAddress) {
+        if (consistentId != null) {
+            return connectionManager.consistentId().equals(consistentId);
         }
 
-        if (cid != null) {
-            return connectionManager.consistentId().equals(cid);
-        }
+        InetSocketAddress localAddress = connectionManager.localAddress();

Review Comment:
   Again, a comment for the future. How can we be sure that there's only one physical address for local node. Everything in this class looks suspicious.



-- 
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