Uploaded image for project: 'Apache Curator'
  1. Apache Curator
  2. CURATOR-402

Curator should not use QuorumServer.addr when updating the connect string dynamically

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Duplicate
    • Affects Version/s: 3.2.1, 3.3.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      https://issues.apache.org/jira/browse/CURATOR-345 made a change to EnsembleTracker to read the QuorumServer.addr in case the QuorumServer.clientAddr field is null. This doesn't seem right to me.

      When the Zookeeper cluster is configured with the new clientPort syntax (http://zookeeper.apache.org/doc/r3.5.3-beta/zookeeperReconfig.html#sc_reconfig_clientport), the clientAddr is correctly set:

      19:54:10.691 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr /0.0.0.0:2181 and server addr localhost/127.0.0.1:2888
      19:54:10.691 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr /0.0.0.0:2182 and server addr localhost/127.0.0.1:2889
      19:54:10.691 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr /0.0.0.0:2183 and server addr localhost/127.0.0.1:2890
      

      If the ensemble is configured with the old clientPort property, the clientAddr fields will be null:

      19:59:23.801 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr null and server addr localhost/127.0.0.1:2888
      19:59:23.801 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr null and server addr localhost/127.0.0.1:2889
      19:59:23.801 [main-EventThread] ERROR org.apache.curator.framework.imps.EnsembleTracker - Getting clientAddr null and server addr localhost/127.0.0.1:2890
      

      Before https://issues.apache.org/jira/browse/CURATOR-345, this was okay, since using the old clientPort property just wound up making the EnsembleTracker.configToConnectionString method return an empty string, which effectively disables Curators ability to update the connection string.

      With the new behavior, the connect string will be updated to the server addresses (i.e. the peer ports) if the clientAddr fields are null, which will result in Curator being unable to reconnect if the Zookeeper instance needs to be replaced.

      As far as I'm aware the client has no use for the Zookeeper peer ports, so the change from https://issues.apache.org/jira/browse/CURATOR-345 should probably be reverted.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user srdo opened a pull request:

          https://github.com/apache/curator/pull/216

          CURATOR-402: Curator should not use QuorumServer.addr when updating the connect string dynamically

          …e the client connect string, resulting in a client unable to reconnect

          See https://issues.apache.org/jira/browse/CURATOR-402.

          I took a look at adding a test for this, but both addresses are null when using a single testing server, and I couldn't find a way to start a testing cluster where clientAddr is null.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/srdo/curator CURATOR-402

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/curator/pull/216.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #216


          commit 942b96368dbd536a61c5cb150bd081a6f0ac2754
          Author: Stig Rohde Døssing <sdo@it-minds.dk>
          Date: 2017-04-24T18:23:41Z

          Fix EnsembleTracker sometimes using the Zookeeper peer ports to update the client connect string, resulting in a client unable to reconnect


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user srdo opened a pull request: https://github.com/apache/curator/pull/216 CURATOR-402 : Curator should not use QuorumServer.addr when updating the connect string dynamically …e the client connect string, resulting in a client unable to reconnect See https://issues.apache.org/jira/browse/CURATOR-402 . I took a look at adding a test for this, but both addresses are null when using a single testing server, and I couldn't find a way to start a testing cluster where clientAddr is null. You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/curator CURATOR-402 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/curator/pull/216.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #216 commit 942b96368dbd536a61c5cb150bd081a6f0ac2754 Author: Stig Rohde Døssing <sdo@it-minds.dk> Date: 2017-04-24T18:23:41Z Fix EnsembleTracker sometimes using the Zookeeper peer ports to update the client connect string, resulting in a client unable to reconnect
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lvfangmin commented on the issue:

          https://github.com/apache/curator/pull/216

          This fix looks good to time, but I'd like @Randgalt to accept it since he has more context about the changes he made in CURATOR-345.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lvfangmin commented on the issue: https://github.com/apache/curator/pull/216 This fix looks good to time, but I'd like @Randgalt to accept it since he has more context about the changes he made in CURATOR-345 .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Randgalt commented on the issue:

          https://github.com/apache/curator/pull/216

          I'll look at this soon. This needs to be addressed once and for all.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Randgalt commented on the issue: https://github.com/apache/curator/pull/216 I'll look at this soon. This needs to be addressed once and for all.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lvfangmin commented on a diff in the pull request:

          https://github.com/apache/curator/pull/216#discussion_r126874138

          — Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java —
          @@ -170,8 +170,10 @@ public static String configToConnectionString(QuorumVerifier data) throws Except

          { sb.append(","); }
          • InetSocketAddress address = Objects.firstNonNull(server.clientAddr, server.addr);
          • sb.append(address.getAddress().getHostAddress()).append(":").append(address.getPort());
            + InetSocketAddress clientAddress = server.clientAddr;
              • End diff –

          After ZooKeeper 3.5, if the zoo.cfg is in old format which has clientPort and server list being defined, then the clientAddr will be 0.0.0.0, which shouldn't be used as the server hostAddress. CURATOR-392 reported some issues similar, and it has handled this case.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lvfangmin commented on a diff in the pull request: https://github.com/apache/curator/pull/216#discussion_r126874138 — Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java — @@ -170,8 +170,10 @@ public static String configToConnectionString(QuorumVerifier data) throws Except { sb.append(","); } InetSocketAddress address = Objects.firstNonNull(server.clientAddr, server.addr); sb.append(address.getAddress().getHostAddress()).append(":").append(address.getPort()); + InetSocketAddress clientAddress = server.clientAddr; End diff – After ZooKeeper 3.5, if the zoo.cfg is in old format which has clientPort and server list being defined, then the clientAddr will be 0.0.0.0, which shouldn't be used as the server hostAddress. CURATOR-392 reported some issues similar, and it has handled this case.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user srdo commented on a diff in the pull request:

          https://github.com/apache/curator/pull/216#discussion_r126888431

          — Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java —
          @@ -170,8 +170,10 @@ public static String configToConnectionString(QuorumVerifier data) throws Except

          { sb.append(","); }
          • InetSocketAddress address = Objects.firstNonNull(server.clientAddr, server.addr);
          • sb.append(address.getAddress().getHostAddress()).append(":").append(address.getPort());
            + InetSocketAddress clientAddress = server.clientAddr;
              • End diff –

          You are right. CURATOR-392 looks like a more complete attempt at fixing this, so this PR doesn't seem necessary.

          Show
          githubbot ASF GitHub Bot added a comment - Github user srdo commented on a diff in the pull request: https://github.com/apache/curator/pull/216#discussion_r126888431 — Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java — @@ -170,8 +170,10 @@ public static String configToConnectionString(QuorumVerifier data) throws Except { sb.append(","); } InetSocketAddress address = Objects.firstNonNull(server.clientAddr, server.addr); sb.append(address.getAddress().getHostAddress()).append(":").append(address.getPort()); + InetSocketAddress clientAddress = server.clientAddr; End diff – You are right. CURATOR-392 looks like a more complete attempt at fixing this, so this PR doesn't seem necessary.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user srdo closed the pull request at:

          https://github.com/apache/curator/pull/216

          Show
          githubbot ASF GitHub Bot added a comment - Github user srdo closed the pull request at: https://github.com/apache/curator/pull/216

            People

            • Assignee:
              randgalt Jordan Zimmerman
              Reporter:
              Srdo Stig Rohde Døssing
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development