Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.5.0
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This issue supersedes our discussion in ZOOKEEPER-1989.

      To summarize, ZK users can seamlessly upgrade 3.4 to 3.5. But two things will happen:
      1. the server list will be separated out as a dynamic file (the original should be backup automatically).
      2. Client port is mandatory on reconfig. So when reconfig the server itself (its id), the client port in config file will be removed and replaced by the one in reconfig (written in dynamic file).

      1. ZOOKEEPER-1992-withspaces.patch
        50 kB
        Hongchao Deng
      2. ZOOKEEPER-1992-without-spaces.patch
        7 kB
        Hongchao Deng
      3. ZOOKEEPER-1992-gitaddp.patch
        13 kB
        Hongchao Deng
      4. ZOOKEEPER-1992-addTest.patch
        26 kB
        Hongchao Deng
      5. final2.patch
        26 kB
        Hongchao Deng
      6. final1.patch
        26 kB
        Alexander Shraer
      7. final.patch
        26 kB
        Hongchao Deng
      8. draft-4.patch
        8 kB
        Hongchao Deng
      9. draft-3.patch
        7 kB
        Hongchao Deng
      10. draft-2.patch
        7 kB
        Hongchao Deng
      11. draft.patch
        5 kB
        Hongchao Deng

        Issue Links

          Activity

          Hide
          Alexander Shraer added a comment -

          > Client port is mandatory on reconfig.

          I think we don't have to enforce this as part of this JIRA.

          How about this:

          -In QuorumPeer.writeDynamicConfig, pass a new parameter that indicates that you need to delete clientPort and clientPortAddress from the static file (last configuration didn't includes a client port for self.getId() but the new one has it, you can know that from where you call this function, in setQuorumVerifier).

          • If a failure happens after updating the dynamic file but before you update the static file, indeed you could come up and have clientPort and/or clientPortAddress in both the static and dynamic files and they could be different. If they are different I think we should still raise an error like we're doing now and require admin intervention. If they are equal you could delete clientPort from the static file in QuorumPeerConfig when the server comes up.

          My rational is that after the server comes up the two files should be consistent and the port is either in the static file or in the dynamic one but not in both. Silently prioritizing the dynamic port and removing the conflicting static port to solve the inconsistency may be a surprise to the user. But actually I don't feel strongly about it. Patrick Hunt, Michi Mutsuzaki which way would you prefer ?

          Show
          Alexander Shraer added a comment - > Client port is mandatory on reconfig. I think we don't have to enforce this as part of this JIRA. How about this: -In QuorumPeer.writeDynamicConfig, pass a new parameter that indicates that you need to delete clientPort and clientPortAddress from the static file (last configuration didn't includes a client port for self.getId() but the new one has it, you can know that from where you call this function, in setQuorumVerifier). If a failure happens after updating the dynamic file but before you update the static file, indeed you could come up and have clientPort and/or clientPortAddress in both the static and dynamic files and they could be different. If they are different I think we should still raise an error like we're doing now and require admin intervention. If they are equal you could delete clientPort from the static file in QuorumPeerConfig when the server comes up. My rational is that after the server comes up the two files should be consistent and the port is either in the static file or in the dynamic one but not in both. Silently prioritizing the dynamic port and removing the conflicting static port to solve the inconsistency may be a surprise to the user. But actually I don't feel strongly about it. Patrick Hunt , Michi Mutsuzaki which way would you prefer ?
          Hide
          Patrick Hunt added a comment -

          If they are different I think we should still raise an error like we're doing now and require admin intervention.

          that sounds reasonable to me

          Show
          Patrick Hunt added a comment - If they are different I think we should still raise an error like we're doing now and require admin intervention. that sounds reasonable to me
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Could you give more details about your method? I will try to make it work.

          Silently prioritizing the dynamic port and removing the conflicting static port to solve the inconsistency may be a surprise to the user.

          As we discussed before, what happened in reconfig client port is that if there is client port in static file, it is gonna remove that one from static file too. But my worry is that this requires much consideration on consistency in removing the line and updating the dynamic file.

          So my plan is that:
          1. Keep the client port on fresh boot for backward compatibility.
          Then what happens if the server reconfig itself client port? Well, I plan for two tasks, where the second task is gonna replace the first one finally.
          2.1 Since it will updates the client port in dynamic file, making a preference on dynamic file over static file is a hackish way to finish it. Not delete/remove here. This would be quick and make out a release. This is part of ZOOKEEPRE-1993
          2.2 (This one is difficult and I need more time to get through the code.) Basically, on reconfiguring the client port on itself, it should safely remove the client port, update the dynamic file. This is part of ZOOKEEPRE-1995

          Does that make sense? Please let me know what you think might be proper solution.

          Show
          Hongchao Deng added a comment - Alexander Shraer Could you give more details about your method? I will try to make it work. Silently prioritizing the dynamic port and removing the conflicting static port to solve the inconsistency may be a surprise to the user. As we discussed before, what happened in reconfig client port is that if there is client port in static file, it is gonna remove that one from static file too. But my worry is that this requires much consideration on consistency in removing the line and updating the dynamic file. So my plan is that: 1. Keep the client port on fresh boot for backward compatibility. Then what happens if the server reconfig itself client port? Well, I plan for two tasks, where the second task is gonna replace the first one finally. 2.1 Since it will updates the client port in dynamic file, making a preference on dynamic file over static file is a hackish way to finish it. Not delete/remove here. This would be quick and make out a release. This is part of ZOOKEEPRE-1993 2.2 (This one is difficult and I need more time to get through the code.) Basically, on reconfiguring the client port on itself, it should safely remove the client port, update the dynamic file. This is part of ZOOKEEPRE-1995 Does that make sense? Please let me know what you think might be proper solution.
          Hide
          Alexander Shraer added a comment -

          Here's my proposal, can you clarify which part you feel uncomfortable with ?

          • in QuorumPeer.writeDynamicConfig, pass a new parameter that indicates that you need to delete clientPort and clientPortAddress from the static file (last configuration didn't includes a client port for self.getId() but the new one has it, you can know that from where you call this function, in QuorumPeer.setQuorumVerifier).
          • in QuorumPeerConfig, where this exception is thrown
            throw new ConfigException("client address for this server (id = " + serverId + ") in static config file is " + clientPortAddress + " is different from client address found in dynamic file: " + qs.clientAddr);

          and an "else" (meaning "everything is consistent) that in case clientPort and clientPortAddress appear in the dynamic file removes them from the static file.

          Show
          Alexander Shraer added a comment - Here's my proposal, can you clarify which part you feel uncomfortable with ? in QuorumPeer.writeDynamicConfig, pass a new parameter that indicates that you need to delete clientPort and clientPortAddress from the static file (last configuration didn't includes a client port for self.getId() but the new one has it, you can know that from where you call this function, in QuorumPeer.setQuorumVerifier). in QuorumPeerConfig, where this exception is thrown throw new ConfigException("client address for this server (id = " + serverId + ") in static config file is " + clientPortAddress + " is different from client address found in dynamic file: " + qs.clientAddr); and an "else" (meaning "everything is consistent) that in case clientPort and clientPortAddress appear in the dynamic file removes them from the static file.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Regarding keeping the client port on fresh boot??
          Just change this??

                                     if (!key.startsWith("server.") && !key.startsWith("group") 
                                             && !key.startsWith("weight") && !key.equals("clientPort") && !key.equals("clientPortAddress")){
                                         out.write(key.concat("=").concat(value).concat("\n"));
                                     }
          
          Show
          Hongchao Deng added a comment - Alexander Shraer Regarding keeping the client port on fresh boot?? Just change this?? if (!key.startsWith( "server." ) && !key.startsWith( "group" ) && !key.startsWith( "weight" ) && !key.equals( "clientPort" ) && !key.equals( "clientPortAddress" )){ out.write(key.concat( "=" ).concat(value).concat( "\n" )); }
          Hide
          Alexander Shraer added a comment -

          right - just remove the "!key.equals("clientPort") && !key.equals("clientPortAddress")" from the above.

          Show
          Alexander Shraer added a comment - right - just remove the "!key.equals("clientPort") && !key.equals("clientPortAddress")" from the above.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Okay.
          I will get the minimal change. As far as I know, I got several test errors If I forced a client port check after parsing.. But I will put off this and delve into the finer grain patches (also later)..

          Show
          Hongchao Deng added a comment - Alexander Shraer Okay. I will get the minimal change. As far as I know, I got several test errors If I forced a client port check after parsing.. But I will put off this and delve into the finer grain patches (also later)..
          Hide
          Hongchao Deng added a comment -

          Hi Alexander Shraer. I am not sure what this means.

          last configuration didn't includes a client port for self.getId() but the new one has it

          Can you be more specific?? Thanks.

          Show
          Hongchao Deng added a comment - Hi Alexander Shraer . I am not sure what this means. last configuration didn't includes a client port for self.getId() but the new one has it Can you be more specific?? Thanks.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Please take a look at the draft for the idea of workflow.
          Let me know your feedback.
          I will try to implement and test them.
          Thanks!

          Show
          Hongchao Deng added a comment - Alexander Shraer Please take a look at the draft for the idea of workflow. Let me know your feedback. I will try to implement and test them. Thanks!
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          I finished the coding. Start testing it. Second draft.

          Show
          Hongchao Deng added a comment - Alexander Shraer I finished the coding. Start testing it. Second draft.
          Hide
          Alexander Shraer added a comment -

          looks pretty good, thanks. a few comments

          • eraseClient() should compare prevQV (the previous committed configuration) and qv or quorumVerifier (the new committed configuration).

          something like eraseClient(QuorumVerier prevQV, QuorumVerifier newQV)

          { QuorumServer myOldSpec = prevQV.getAllMembers().get(getId()); QuorumServer myNewSpec = quorumVerifier.getAllMembers().get(getId()); return (myNewSpec != null && myNewSpec.clientAddr != null && (myOldSpec == null || myOldSpec.clientAddr == null); }

          maybe change "eraseClient" to something like "eraseStaticClientPortInfo" so that its more clear

          Show
          Alexander Shraer added a comment - looks pretty good, thanks. a few comments eraseClient() should compare prevQV (the previous committed configuration) and qv or quorumVerifier (the new committed configuration). something like eraseClient(QuorumVerier prevQV, QuorumVerifier newQV) { QuorumServer myOldSpec = prevQV.getAllMembers().get(getId()); QuorumServer myNewSpec = quorumVerifier.getAllMembers().get(getId()); return (myNewSpec != null && myNewSpec.clientAddr != null && (myOldSpec == null || myOldSpec.clientAddr == null); } maybe change "eraseClient" to something like "eraseStaticClientPortInfo" so that its more clear
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          One more question:
          I am confused by setLastSeenQuorumXXX and setQuorumXXX.
          I think I just need erase client port in setQuorumXXX because that's where the client port is updated to actual dynamic file. Correct?

          Show
          Hongchao Deng added a comment - Alexander Shraer One more question: I am confused by setLastSeenQuorumXXX and setQuorumXXX. I think I just need erase client port in setQuorumXXX because that's where the client port is updated to actual dynamic file. Correct?
          Hide
          Alexander Shraer added a comment -

          yes, that's correct.
          the logic here seems wrong - some of the && should be || etc... a key can't both start with server. and with group.

          + if ((backwardCompatible
          + && key.startsWith("server.") && key.startsWith("group")
          + && key.startsWith("weight"))
          + ||
          + (eraseClient
          + && key.equals("clientPort") && key.equals("clientPortAddress"))) {
          + continue;

          if (!key.startsWith("server.") && !key.startsWith("group") && !key.startsWith("weight")
          && (!eraseClient || ( !key.equals("clientPort") && !key.equals("clientPortAddress"))){
          out.write(key.concat("=").concat(value).concat("\n"));

          Show
          Alexander Shraer added a comment - yes, that's correct. the logic here seems wrong - some of the && should be || etc... a key can't both start with server. and with group. + if ((backwardCompatible + && key.startsWith("server.") && key.startsWith("group") + && key.startsWith("weight")) + || + (eraseClient + && key.equals("clientPort") && key.equals("clientPortAddress"))) { + continue; if (!key.startsWith("server.") && !key.startsWith("group") && !key.startsWith("weight") && (!eraseClient || ( !key.equals("clientPort") && !key.equals("clientPortAddress"))){ out.write(key.concat("=").concat(value).concat("\n"));
          Hide
          Alexander Shraer added a comment -

          sorry - the "if" at the end of my message is a suggested change

          Show
          Alexander Shraer added a comment - sorry - the "if" at the end of my message is a suggested change
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          There you go~ I will open a review board start testing it

          Show
          Hongchao Deng added a comment - Alexander Shraer There you go~ I will open a review board start testing it
          Hide
          Hongchao Deng added a comment -

          Hi Alexander Shraer
          I uploaded the final patch that I tested manually:
          1. Start from old style config, has client port remain
          2. reconfig change port, port is removed from static file and added in dynamic file.

          I have a few unit tests failing. I am trying to look at them and ask for more help.
          I have a question about reconfig client port:
          1. If a client reconfig the port it's connected to, what happened? Is it automatically changed to another port?

          Show
          Hongchao Deng added a comment - Hi Alexander Shraer I uploaded the final patch that I tested manually: 1. Start from old style config, has client port remain 2. reconfig change port, port is removed from static file and added in dynamic file. I have a few unit tests failing. I am trying to look at them and ask for more help. I have a question about reconfig client port: 1. If a client reconfig the port it's connected to, what happened? Is it automatically changed to another port?
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          I couldn't get those annoying whitespace changes out on reviewboard. So I am using Github for it

          https://github.com/fengjingchao/zookeeper/commit/ab49b98ca07d2a675115b4a65a552e64c544127d?w=1

          Show
          Hongchao Deng added a comment - Alexander Shraer I couldn't get those annoying whitespace changes out on reviewboard. So I am using Github for it https://github.com/fengjingchao/zookeeper/commit/ab49b98ca07d2a675115b4a65a552e64c544127d?w=1
          Show
          Hongchao Deng added a comment - Updated the patch. Review place: https://github.com/fengjingchao/zookeeper/commit/4528d636d0639b62fcabe2ec07a0ac276fe967cc?w=1
          Hide
          Hongchao Deng added a comment -

          The previous patch is generated using
          "git diff -w"
          which ignores whitespaces.
          In case that doesn't work, here's an equivalent patch which contains all whitespace, CRLT->LF changes using
          "git diff"

          Show
          Hongchao Deng added a comment - The previous patch is generated using "git diff -w" which ignores whitespaces. In case that doesn't work, here's an equivalent patch which contains all whitespace, CRLT->LF changes using "git diff"
          Hide
          Hongchao Deng added a comment -

          This patch is generated using
          "git diff -w --no-prefix"
          The previous "-withspaces" one is
          "git diff --no-prefix"

          Show
          Hongchao Deng added a comment - This patch is generated using "git diff -w --no-prefix" The previous "-withspaces" one is "git diff --no-prefix"
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12658790/ZOOKEEPER-1992-withspaces.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12658790/ZOOKEEPER-1992-withspaces.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2247//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12658791/ZOOKEEPER-1992-without-spaces.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2248//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12658791/ZOOKEEPER-1992-without-spaces.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2248//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          I do a
          "git add -p"
          And this is the minimal changes I picked.

          Show
          Hongchao Deng added a comment - I do a "git add -p" And this is the minimal changes I picked.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          This is the latest patch from the "ZOOKEEPER-1992-gitaddp.patch":

          https://github.com/fengjingchao/zookeeper/commit/217b7de2b242efbf6bb9dd85baad02b34dfd316e?w=1

          When ignoring whitespace/line-ending changes they are all the same. It's the as minimal as I could. Hopefully we can get this in.

          Show
          Hongchao Deng added a comment - Alexander Shraer This is the latest patch from the " ZOOKEEPER-1992 -gitaddp.patch": https://github.com/fengjingchao/zookeeper/commit/217b7de2b242efbf6bb9dd85baad02b34dfd316e?w=1 When ignoring whitespace/line-ending changes they are all the same. It's the as minimal as I could. Hopefully we can get this in.
          Hide
          Alexander Shraer added a comment -

          Hi, Some test is failing - see last test results (ReconfigTest.testRemoveAddOne)

          Show
          Alexander Shraer added a comment - Hi, Some test is failing - see last test results (ReconfigTest.testRemoveAddOne)
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          I saw that this afternoon. It's weird..
          I tested it locally and in my jenkins. They all passed.
          Let it trigger another Pre-Build and see what happened.

          Show
          Hongchao Deng added a comment - Alexander Shraer I saw that this afternoon. It's weird.. I tested it locally and in my jenkins. They all passed. Let it trigger another Pre-Build and see what happened.
          Hide
          Alexander Shraer added a comment -

          Hongchao Deng can you attach an uptodate patch generated by svn diff including the new test(s) ?

          Show
          Alexander Shraer added a comment - Hongchao Deng can you attach an uptodate patch generated by svn diff including the new test(s) ?
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Sure.
          Just sync up before I wrote the code.

          Regarding the tests, I plan for the following:
          1. test dynamic goes up with old style config will have client port and can do reconfig
          2. test reconfig client port itself will remove the client port in static file and add it in dynamic file.

          Let me know if I miss anything.

          Show
          Hongchao Deng added a comment - Alexander Shraer Sure. Just sync up before I wrote the code. Regarding the tests, I plan for the following: 1. test dynamic goes up with old style config will have client port and can do reconfig 2. test reconfig client port itself will remove the client port in static file and add it in dynamic file. Let me know if I miss anything.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12658827/ZOOKEEPER-1992-gitaddp.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12658827/ZOOKEEPER-1992-gitaddp.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2249//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          when you do the reconfig in a test, for example adding one of the servers this time with a port specified, you could check that its static config was updated (like you suggest) and also check that the static configs of the other servers were not updated - still contain their client ports.

          Show
          Alexander Shraer added a comment - when you do the reconfig in a test, for example adding one of the servers this time with a port specified, you could check that its static config was updated (like you suggest) and also check that the static configs of the other servers were not updated - still contain their client ports.
          Hide
          Alexander Shraer added a comment -

          some minor naming suggestions:

          • needEraseClient() -> eraseClientInfoFromStaticConfig()
          • eraseLinesInStaticFile() - > editStaticConfig () (since it is also adding lines, not only removing)
          Show
          Alexander Shraer added a comment - some minor naming suggestions: needEraseClient() -> eraseClientInfoFromStaticConfig() eraseLinesInStaticFile() - > editStaticConfig () (since it is also adding lines, not only removing)
          Hide
          Alexander Shraer added a comment -

          if you look at the log for the failed test, there is a null pointer exception. It happens because the test doesn't create a static config file - it creates a QuorumPeer object directly and has only a dynamic config file. To fix this change
          QuorumPeer.needEraseClient last line to "return (configFilename!=null && ... "

          > If a client reconfig the port it's connected to, what happened? Is it automatically changed to another port?

          I think right now it just closes the socket and the clients disconnect. I'm not 100% sure. There may be a better way to do it, if you'd like, take a look at NIOServerCnxmFactory.java, reconfigure() and NettyServerCnxnFactory.java reconfigure()

          Show
          Alexander Shraer added a comment - if you look at the log for the failed test, there is a null pointer exception. It happens because the test doesn't create a static config file - it creates a QuorumPeer object directly and has only a dynamic config file. To fix this change QuorumPeer.needEraseClient last line to "return (configFilename!=null && ... " > If a client reconfig the port it's connected to, what happened? Is it automatically changed to another port? I think right now it just closes the socket and the clients disconnect. I'm not 100% sure. There may be a better way to do it, if you'd like, take a look at NIOServerCnxmFactory.java, reconfigure() and NettyServerCnxnFactory.java reconfigure()
          Hide
          Alexander Shraer added a comment -

          sorry, I take it back - with the current implementation existing clients won't disconnect, new ones trying to connect will have to use the new client port.

          Show
          Alexander Shraer added a comment - sorry, I take it back - with the current implementation existing clients won't disconnect, new ones trying to connect will have to use the new client port.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          editStaticConfig sounds better. I will change it.

          if you look at the log for the failed test, there is a null pointer exception. It happens because the test doesn't create a static config file - it creates a QuorumPeer object directly and has only a dynamic config file. To fix this change
          QuorumPeer.needEraseClient last line to "return (configFilename!=null && ... "

          The flow is:

          needEraseClient => writeDynamicConfig => editStaticConfig
          

          editStaticConfig is the only thing using configFile. Is it better to prevent that in

              private static void editStaticConfig(final String configFileStr,
                                                  ......
                  // some tests may not have static config file
                  if (configFileStr == null)
                      return;
          
                  File configFile = (new VerifyingFileFactory.Builder(LOG)
                          .warnForRelativePath()
                          .failForNonExistingPath()
                          .build()).create(configFileStr);
          
          Show
          Hongchao Deng added a comment - Alexander Shraer editStaticConfig sounds better. I will change it. if you look at the log for the failed test, there is a null pointer exception. It happens because the test doesn't create a static config file - it creates a QuorumPeer object directly and has only a dynamic config file. To fix this change QuorumPeer.needEraseClient last line to "return (configFilename!=null && ... " The flow is: needEraseClient => writeDynamicConfig => editStaticConfig editStaticConfig is the only thing using configFile. Is it better to prevent that in private static void editStaticConfig( final String configFileStr, ...... // some tests may not have static config file if (configFileStr == null ) return ; File configFile = ( new VerifyingFileFactory.Builder(LOG) .warnForRelativePath() .failForNonExistingPath() .build()).create(configFileStr);
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer

          One more thing, Can I change DynamicConfigBackwardCompatibilityTest.java to ReconfigBackwardCompatibilityTest.java or ReconfigLegacyTest.java

          1. name consistency: All reconfig tests start with "Reconfig", people jumping to the code can find relevant tests.

          Show
          Hongchao Deng added a comment - Alexander Shraer One more thing, Can I change DynamicConfigBackwardCompatibilityTest.java to ReconfigBackwardCompatibilityTest.java or ReconfigLegacyTest.java 1. name consistency: All reconfig tests start with "Reconfig", people jumping to the code can find relevant tests.
          Hide
          Alexander Shraer added a comment -

          sure, both things sound fine

          Show
          Alexander Shraer added a comment - sure, both things sound fine
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer

          with the current implementation existing clients won't disconnect

          On a client port change, if it won't disconnect, will it still use the old port or redirect to another?
          The current cli tool still shows the old port.

          [zk: 127.0.0.1:2181(CONNECTED)
          
          Show
          Hongchao Deng added a comment - Alexander Shraer with the current implementation existing clients won't disconnect On a client port change, if it won't disconnect, will it still use the old port or redirect to another? The current cli tool still shows the old port. [zk: 127.0.0.1:2181(CONNECTED)
          Hide
          Alexander Shraer added a comment -

          on accept() a server socket returns a new socket which the newly connected client and the server use. So as I understand it, closing the server socket doesn't kill the existing connections - they are using different sockets.

          Show
          Alexander Shraer added a comment - on accept() a server socket returns a new socket which the newly connected client and the server use. So as I understand it, closing the server socket doesn't kill the existing connections - they are using different sockets.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          I finished it and uploaded the new patch.

          As usual, review place:
          https://github.com/fengjingchao/zookeeper/commit/621f9f641945f4b9d7050c41bf9a2069baf1d582?w=1

          I also added the check on editing static file.

          on accept() a server socket returns a new socket which the newly connected client and the server use. So as I understand it, closing the server socket doesn't kill the existing connections - they are using different sockets.

          So I think on reconfig a client will be "redirected" to a new server port. This is not shown in cli tool, I mean.

          Show
          Hongchao Deng added a comment - Alexander Shraer I finished it and uploaded the new patch. As usual, review place: https://github.com/fengjingchao/zookeeper/commit/621f9f641945f4b9d7050c41bf9a2069baf1d582?w=1 I also added the check on editing static file. on accept() a server socket returns a new socket which the newly connected client and the server use. So as I understand it, closing the server socket doesn't kill the existing connections - they are using different sockets. So I think on reconfig a client will be "redirected" to a new server port. This is not shown in cli tool, I mean.
          Hide
          Alexander Shraer added a comment -

          Its not redirected upon reconfig. It just continues to use the socket that was created when it first connected, which is different from the server socket that is closed. Currently the client is not notified. It can find out as usual by subscribing to configuration change notifications. I'm not sure what's the right thing to present on the CLI tool, but not sure its that important.

          Show
          Alexander Shraer added a comment - Its not redirected upon reconfig. It just continues to use the socket that was created when it first connected, which is different from the server socket that is closed. Currently the client is not notified. It can find out as usual by subscribing to configuration change notifications. I'm not sure what's the right thing to present on the CLI tool, but not sure its that important.
          Hide
          Hongchao Deng added a comment -

          This adds a retry logic to test if clientPort has been removed.
          Because after reconfig succeeded, there might be race that it setQuorumVerifier.

          It's the same as "ZOOKEEPER-1992-addTest.patch" except the retry logic.

          Show
          Hongchao Deng added a comment - This adds a retry logic to test if clientPort has been removed. Because after reconfig succeeded, there might be race that it setQuorumVerifier. It's the same as " ZOOKEEPER-1992 -addTest.patch" except the retry logic.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12658951/ZOOKEEPER-1992-addTest.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12658951/ZOOKEEPER-1992-addTest.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2251//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          I remove my new retry logic.

          So the latest patch is ZOOKEEPER-1992-addTest.patch

          Above it shows the test has passed.
          Alexander Shraer

          Show
          Hongchao Deng added a comment - I remove my new retry logic. So the latest patch is ZOOKEEPER-1992 -addTest.patch Above it shows the test has passed. Alexander Shraer
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Okay. I finalized everything into "final.patch".

          The previous "ZOOKEEPER-1992-addTest.patch" is flaky because there might be race after it finish reconfig before it finish editing static file the assertFalse on "clientPort" failed. I mitigate the pain by moving the check to the last.

          An ideal solution is that there is a way to notify outside once editing static file is done.

          Show
          Hongchao Deng added a comment - Alexander Shraer Okay. I finalized everything into "final.patch". The previous " ZOOKEEPER-1992 -addTest.patch" is flaky because there might be race after it finish reconfig before it finish editing static file the assertFalse on "clientPort" failed. I mitigate the pain by moving the check to the last. An ideal solution is that there is a way to notify outside once editing static file is done.
          Hide
          Alexander Shraer added a comment -

          +1 The patch looks good, thanks for the hard work Hongchao!
          I made a few minor edits, mainly variable names and some changes to the test. Lets see that final1.patch passes the tests on Jenkins and then commit it.

          Show
          Alexander Shraer added a comment - +1 The patch looks good, thanks for the hard work Hongchao! I made a few minor edits, mainly variable names and some changes to the test. Lets see that final1.patch passes the tests on Jenkins and then commit it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12658991/final.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12658991/final.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2252//console This message is automatically generated.
          Hide
          Hongchao Deng added a comment -

          Alexander Shraer
          Just minor changes.
          1. Code formatting in editStaticConfig. Previously it wasn't aligned after name change (eraseLinesXXX -> editStaticXX).
          2. test case:

          final int changedServerId = SERVER_COUNT - 1;
          

          To check it last to mitigate flaky case.

          Show
          Hongchao Deng added a comment - Alexander Shraer Just minor changes. 1. Code formatting in editStaticConfig. Previously it wasn't aligned after name change (eraseLinesXXX -> editStaticXX). 2. test case: final int changedServerId = SERVER_COUNT - 1; To check it last to mitigate flaky case.
          Hide
          Hongchao Deng added a comment -

          Have it as final2.patch

          Show
          Hongchao Deng added a comment - Have it as final2.patch
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12659081/final2.patch
          against trunk revision 1613553.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12659081/final2.patch against trunk revision 1613553. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2253//console This message is automatically generated.
          Hide
          Alexander Shraer added a comment -

          Committed final1.patch + reformatting as suggested by Hongchao + small change to test so that it waits for 1 sec before checking the files. I chose that over setting the changedServerId to 2 since we also want to check the configuration files of the other 2 servers.

          Show
          Alexander Shraer added a comment - Committed final1.patch + reformatting as suggested by Hongchao + small change to test so that it waits for 1 sec before checking the files. I chose that over setting the changedServerId to 2 since we also want to check the configuration files of the other 2 servers.
          Hide
          Alexander Shraer added a comment -

          Committed to trunk. Thanks Hongchao!

          Show
          Alexander Shraer added a comment - Committed to trunk. Thanks Hongchao!
          Hide
          Hudson added a comment -

          FAILURE: Integrated in ZooKeeper-trunk #2393 (See https://builds.apache.org/job/ZooKeeper-trunk/2393/)
          ZOOKEEPER-1992. Backward compatibility of the static configuration file (Hongchao Deng via Alex Shraer) (shralex: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615037)

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/DynamicConfigBackwardCompatibilityTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java
          Show
          Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2393 (See https://builds.apache.org/job/ZooKeeper-trunk/2393/ ) ZOOKEEPER-1992 . Backward compatibility of the static configuration file (Hongchao Deng via Alex Shraer) (shralex: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615037 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/DynamicConfigBackwardCompatibilityTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java
          Hide
          Alexander Shraer added a comment -

          I just noticed that we have a bug which I missed - when invoking editStaticConfig from QuorumPeerConfig it will remove the server. weight and other dynamic definitions even though the dynamic file doesn't exist yet. Assuming that ZOOKEEPER-1994 will get in 3.5.0, lets fix it there.

          Show
          Alexander Shraer added a comment - I just noticed that we have a bug which I missed - when invoking editStaticConfig from QuorumPeerConfig it will remove the server. weight and other dynamic definitions even though the dynamic file doesn't exist yet. Assuming that ZOOKEEPER-1994 will get in 3.5.0, lets fix it there.

            People

            • Assignee:
              Hongchao Deng
              Reporter:
              Hongchao Deng
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development