[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3468: ARTEMIS-3138 Shared Nothing Live broker shouldn't try to connect to itself

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

franz1981 commented on a change in pull request #3468:
URL: https://github.com/apache/activemq-artemis/pull/3468#discussion_r582625856



##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java
##########
@@ -1546,20 +1546,22 @@ public synchronized void connectorsChanged(List<DiscoveryEntry> newConnectors) {
       if (receivedTopology) {
          return;
       }
-      TransportConfiguration[] newInitialconnectors = (TransportConfiguration[]) Array.newInstance(TransportConfiguration.class, newConnectors.size());
 
-      int count = 0;
-      for (DiscoveryEntry entry : newConnectors) {
-         newInitialconnectors[count++] = entry.getConnector();
+      final List<TransportConfiguration> newInitialconnectors = new ArrayList<>(newConnectors.size());
 
+      for (DiscoveryEntry entry : newConnectors) {
          if (ha && topology.getMember(entry.getNodeID()) == null) {
             TopologyMemberImpl member = new TopologyMemberImpl(entry.getNodeID(), null, null, entry.getConnector(), null);
             // on this case we set it as zero as any update coming from server should be accepted
             topology.updateMember(0, entry.getNodeID(), member);
          }
+         // ignore its own transport connector
+         if (!entry.getConnector().equals(clusterTransportConfiguration)) {

Review comment:
       > As long as your own connection is getting into the Topology for the clients, this is ok.
   
   That's why the check `entry.getConnector().equals(clusterTransportConfiguration)` is placed after updating `topology`: the new `entry` should affect the topology. but not the `initialConnectors` list, because the latter would make it create a connection to itself ie is not a valid connector.
   
   I'm just concerned that the we throw multiple times `ActiveMQClientMessageBundle.BUNDLE.noTCForSessionFactory()` that's a `ActiveMQException` and it will cause the `ConnectRunnable` to re-schedule itself on `retry interval`, see:
   ```java
      /**
       * used for making the initial connection in the cluster
       */
      private final class ConnectRunnable implements Runnable {
   
         private final ServerLocatorInternal serverLocator;
   
         private ConnectRunnable(ServerLocatorInternal serverLocator) {
            this.serverLocator = serverLocator;
         }
   
         @Override
         public void run() {
            try {
               if (started) {
                  serverLocator.connect();
                  if (serverLocator == replicationLocator) {
                     replicationClusterConnectedLatch.countDown();
                  }
               }
            } catch (ActiveMQException e) {
               if (!started) {
                  return;
               }
               if (logger.isDebugEnabled()) {
   
                  logger.debug("retry on Cluster Controller " + System.identityHashCode(ClusterController.this) + " server = " + server);
               }
               server.getScheduledPool().schedule(this, serverLocator.getRetryInterval(), TimeUnit.MILLISECONDS);
            }
         }
      }
   ```
   Throwing many exceptions isn't nice and I see that it will keep on doing it during the whole life-cycle of a live server *if* there is a single pair, because the backup won't have any acceptor opened to allow the live to connect to it...




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org