ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1346

Handle 4lws and monitoring on separate port

    Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: server
    • Labels:
      None

      Description

      Move the 4lws to their own port, off of the client port, and support them properly via long-lived sessions instead of polling. Deprecate the 4lw support on the client port. Will enable us to enhance the functionality of the commands via extended command syntax, address security concerns and fix bugs involving the socket close being received before all of the data on the client end.

      1. ZOOKEEPER-1346_jetty.patch
        17 kB
        Skye Wanderman-Milne
      2. ZOOKEEPER-1346.2.patch
        71 kB
        Skye Wanderman-Milne
      3. ZOOKEEPER-1346.3.patch
        99 kB
        Skye Wanderman-Milne
      4. ZOOKEEPER-1346.4.patch
        131 kB
        Skye Wanderman-Milne
      5. ZOOKEEPER-1346.6.patch
        130 kB
        Skye Wanderman-Milne
      6. ZOOKEEPER-1346.7.patch
        140 kB
        Skye Wanderman-Milne
      7. ZOOKEEPER-1346.8.patch
        140 kB
        Skye Wanderman-Milne
      8. ZOOKEEPER-1346.patch
        50 kB
        Skye Wanderman-Milne

        Issue Links

          Activity

          Hide
          Skye Wanderman-Milne added a comment -

          What if we replace the current 4lw protocol with a Jetty server? Benefits include:

          • http fixes ZOOKEEPER-1197
          • We could serve up status pages in JSON, XML, HTML, ... (this will make integrating with third party tools easier too)
          • Adds support for encryption + authentication ( => potential for more exciting commands)
          • We can keep the old TCP protocol as well for backwards compatibility
          • Resolves this ticket

          I've written a rough prototype implementation, see ZOOKEEPER-1346_jetty.patch. Basically the patch copies the 4lw commands from the ServerCnxn classes into a new Commands class, which is used by a new JettyServer class. To try it out start ZK and go to e.g. http://localhost:8080/command?cmd=stats.

          Show
          Skye Wanderman-Milne added a comment - What if we replace the current 4lw protocol with a Jetty server? Benefits include: http fixes ZOOKEEPER-1197 We could serve up status pages in JSON, XML, HTML, ... (this will make integrating with third party tools easier too) Adds support for encryption + authentication ( => potential for more exciting commands) We can keep the old TCP protocol as well for backwards compatibility Resolves this ticket I've written a rough prototype implementation, see ZOOKEEPER-1346 _jetty.patch. Basically the patch copies the 4lw commands from the ServerCnxn classes into a new Commands class, which is used by a new JettyServer class. To try it out start ZK and go to e.g. http://localhost:8080/command?cmd=stats .
          Hide
          Patrick Hunt added a comment -

          Sounds like a great idea to me. I've seen a number of issues now with users experiencing this problem. Also the fact that some users don't like that the monitoring port is the same as the session port. They'd like to segregate/firewall this off (the 4lw from user of the zk service itself).

          If we do add this it would be good if there was a way to include it optionally. Some projects integrate ZK into their system (think embedded zk, e.g. hbase) and have their own container requirements. Similarly some users may not want to include jetty dependencies/runtime for whatever reason.

          Given the rest of the hadoop ecosystem typically has similar functionality (jetty based http/s interfaces) it doesn't seem like we're going to far afield here.

          Show
          Patrick Hunt added a comment - Sounds like a great idea to me. I've seen a number of issues now with users experiencing this problem. Also the fact that some users don't like that the monitoring port is the same as the session port. They'd like to segregate/firewall this off (the 4lw from user of the zk service itself). If we do add this it would be good if there was a way to include it optionally. Some projects integrate ZK into their system (think embedded zk, e.g. hbase) and have their own container requirements. Similarly some users may not want to include jetty dependencies/runtime for whatever reason. Given the rest of the hadoop ecosystem typically has similar functionality (jetty based http/s interfaces) it doesn't seem like we're going to far afield here.
          Hide
          Henry Robinson added a comment -

          +1, great idea.

          I do think it's important that we set out with the intention of deprecating the old protocol eventually. This is a good opportunity to properly establish a procedure for doing that. I suggest including both in the next major release (3.5?) and warning that the old protocol will be turned off in 3.6. Assuming all goes according to plan, we can eventually ship 3.6 with only a Jetty-based 4lw implementation.

          Show
          Henry Robinson added a comment - +1, great idea. I do think it's important that we set out with the intention of deprecating the old protocol eventually. This is a good opportunity to properly establish a procedure for doing that. I suggest including both in the next major release (3.5?) and warning that the old protocol will be turned off in 3.6. Assuming all goes according to plan, we can eventually ship 3.6 with only a Jetty-based 4lw implementation.
          Hide
          Camille Fournier added a comment -

          Thumbs-up from me, sounds like a good idea. Skye I'm gonna bump this ticket to you but I'm happy to CR changes along the way as needed.

          Show
          Camille Fournier added a comment - Thumbs-up from me, sounds like a good idea. Skye I'm gonna bump this ticket to you but I'm happy to CR changes along the way as needed.
          Hide
          Skye Wanderman-Milne added a comment -

          This patch implements the main functionality of the new server, although there is still work to do before this is ready to be committed.

          DESIGN SUMMARY
          This patch introduces a new ZK server package, org.apache.zookeeper.server.admin. This package includes Commands.java, which reimplements the 4lw commands from

          {Nio,Netty}ServerCnxn. Each new command implements the Command interface, which includes a run method that takes a Map<String, String> of keyword arguments. This makes it easy to run a command with arguments specified as URL parameters. Command.run returns a Map<String, Object>, which at minimum contains a "command" key for identifying which command was run and a possibly-null "error" key. Commands are expected to use the "error" entry instead of raising exceptions so as to inform users of errors in a consistent and clear way (uncaught exceptions, i.e. programming errors, are currently handled automatically by Jetty – it returns a 500 status code with the error including the stacktrace and logs the error and stacktrace).

          Commands.java also has static methods for registering commands and running a registered command given its name, which we use instead of creating and invoking a Command instance directly. This takes care of defining available commands and mapping names to Commands. Right now the 4lw commands are registered in a static block in Commands.java (in the future we can allow for pluggable commands too).

          The package also includes AdminServer.java, which starts a Jetty server that runs commands via Commands.java. Commands are run by loading a certain URL, e.g., http://localhost:8080/command/stat. Any arguments are specified as URL parameters, e.g., http://localhost:8080/command/set_trace_mask?traceMask=300. The result of a Command is displayed using a CommandOutputter, which is responsible for outputting the Map<String, Object> returned by a Command to a PrintWriter. For now I've only included a JsonOutputter for simplicity, but I'm also working on a TextOutputter that reproduces the output of the current 4lws.

          Besides adding the new .admin package, I added a bunch of methods that basically duplicate methods used by the current 4lws except return a Map instead of writing to a PrintWriter. I also had to make a few changes since the new commands are in a separate package, unlike the current 4lws.

          I did not remove or change any of the current 4lw code to make this patch easier to review. However, we may want to refactor the 4lw commands in {Nio,Netty}

          ServerCnxn to use Commands.java (more below).

          NEXT STEPS
          Right now an AdminServer is only started in standalone mode, obviously we should also start one for quorum peers.

          TextOutputter

          QUESTIONS
          Should loading the embedded server be optional? If so, what's the story for the current 4lw commands? It's very appealing to deprecate and eventually remove them, but that would mean removing the 4lw functionality altogether for users who opt not to include the embedded server. If we choose to keep the current 4lw commands, I'll refactor them to use the Commands class (this means we're still stuck with all the complexity and bugs, including this one).

          If loading the server is optional, what should that look like? There should probably be a configuration parameter. It might also be nice to load the server via reflection so users can remove Jetty from the classpath altogether.

          Any thoughts on error handling?

          Any thoughts on pluggable commands?

          Show
          Skye Wanderman-Milne added a comment - This patch implements the main functionality of the new server, although there is still work to do before this is ready to be committed. DESIGN SUMMARY This patch introduces a new ZK server package, org.apache.zookeeper.server.admin. This package includes Commands.java, which reimplements the 4lw commands from {Nio,Netty}ServerCnxn. Each new command implements the Command interface, which includes a run method that takes a Map<String, String> of keyword arguments. This makes it easy to run a command with arguments specified as URL parameters. Command.run returns a Map<String, Object>, which at minimum contains a "command" key for identifying which command was run and a possibly-null "error" key. Commands are expected to use the "error" entry instead of raising exceptions so as to inform users of errors in a consistent and clear way (uncaught exceptions, i.e. programming errors, are currently handled automatically by Jetty – it returns a 500 status code with the error including the stacktrace and logs the error and stacktrace). Commands.java also has static methods for registering commands and running a registered command given its name, which we use instead of creating and invoking a Command instance directly. This takes care of defining available commands and mapping names to Commands. Right now the 4lw commands are registered in a static block in Commands.java (in the future we can allow for pluggable commands too). The package also includes AdminServer.java, which starts a Jetty server that runs commands via Commands.java. Commands are run by loading a certain URL, e.g., http://localhost:8080/command/stat . Any arguments are specified as URL parameters, e.g., http://localhost:8080/command/set_trace_mask?traceMask=300 . The result of a Command is displayed using a CommandOutputter, which is responsible for outputting the Map<String, Object> returned by a Command to a PrintWriter. For now I've only included a JsonOutputter for simplicity, but I'm also working on a TextOutputter that reproduces the output of the current 4lws. Besides adding the new .admin package, I added a bunch of methods that basically duplicate methods used by the current 4lws except return a Map instead of writing to a PrintWriter. I also had to make a few changes since the new commands are in a separate package, unlike the current 4lws. I did not remove or change any of the current 4lw code to make this patch easier to review. However, we may want to refactor the 4lw commands in {Nio,Netty} ServerCnxn to use Commands.java (more below). NEXT STEPS Right now an AdminServer is only started in standalone mode, obviously we should also start one for quorum peers. TextOutputter QUESTIONS Should loading the embedded server be optional? If so, what's the story for the current 4lw commands? It's very appealing to deprecate and eventually remove them, but that would mean removing the 4lw functionality altogether for users who opt not to include the embedded server. If we choose to keep the current 4lw commands, I'll refactor them to use the Commands class (this means we're still stuck with all the complexity and bugs, including this one). If loading the server is optional, what should that look like? There should probably be a configuration parameter. It might also be nice to load the server via reflection so users can remove Jetty from the classpath altogether. Any thoughts on error handling? Any thoughts on pluggable commands?
          Show
          Skye Wanderman-Milne added a comment - rb: https://reviews.apache.org/r/8094/
          Hide
          Ted Dunning added a comment -

          Skye,

          Nice work. Is there documentation about what URL structure you used?

          Is there a way to make it a bit more REST-y?

          Are there commands that are natural to decompose a bit using a hierarchical URL style? For instance, if have something that lists live quorum members, it would be nice be able to append the quorum member id to the URL to get more details on that member alone. Likewise for live sessions.

          Further afield, are there metrics that should be reported/set using this mechanism? I know that we report a fair number of metrics using JMX, but would it be good to have a different mechanism?

          Show
          Ted Dunning added a comment - Skye, Nice work. Is there documentation about what URL structure you used? Is there a way to make it a bit more REST-y? Are there commands that are natural to decompose a bit using a hierarchical URL style? For instance, if have something that lists live quorum members, it would be nice be able to append the quorum member id to the URL to get more details on that member alone. Likewise for live sessions. Further afield, are there metrics that should be reported/set using this mechanism? I know that we report a fair number of metrics using JMX, but would it be good to have a different mechanism?
          Hide
          Skye Wanderman-Milne added a comment -

          The javadoc for the AdminServer class goes over the URLs, but this should probably be in the actual docs too. I'll update them once everything is finalized. There's really only two URLs right now: /command (lists all the commands), and /command/<command> (runs <command>). I'm open to suggestions for how to change this, including how to make it more REST-y

          I like the idea of supporting further URL hierarchies. This would probably require changing how the server turns URLs into Commands... maybe a command can be registered with a name like "session/*" or "session/<id>" and the server can fill in what was actually requested? I would have to think about it. This is probably something to work on after all the basic functionality is implemented to make sure we don't complicate things too much.

          Right now I'm focusing on reimplementing the current 4lw functionality, but I definitely would like to add new functionality, including the ability to make changes to the ZK server. I'll check out what the JMX metrics are. On this note, I'd also like to support authentication and encryption (again, after the basics are in place).

          Show
          Skye Wanderman-Milne added a comment - The javadoc for the AdminServer class goes over the URLs, but this should probably be in the actual docs too. I'll update them once everything is finalized. There's really only two URLs right now: /command (lists all the commands), and /command/<command> (runs <command>). I'm open to suggestions for how to change this, including how to make it more REST-y I like the idea of supporting further URL hierarchies. This would probably require changing how the server turns URLs into Commands... maybe a command can be registered with a name like "session/*" or "session/<id>" and the server can fill in what was actually requested? I would have to think about it. This is probably something to work on after all the basic functionality is implemented to make sure we don't complicate things too much. Right now I'm focusing on reimplementing the current 4lw functionality, but I definitely would like to add new functionality, including the ability to make changes to the ZK server. I'll check out what the JMX metrics are. On this note, I'd also like to support authentication and encryption (again, after the basics are in place).
          Hide
          Nikita Vetoshkin added a comment -

          About ReST'inness. I think it should be better to prefix this API with /v1/api from the start. In ReST with URLs you should provide hierarchy of resources. In our case we have a collection of command*s* which can look like this:

          commands/
          |-- configuration
          |-- connections
          |-- connection_stat_reset
          |-- dump
          |-- environment
          |-- get_trace_mask
          `-- monitor
          

          Then, to get the environment one must issue a GET request to the URL /v1/api/commands/environment.

          Show
          Nikita Vetoshkin added a comment - About ReST'inness. I think it should be better to prefix this API with /v1/api from the start. In ReST with URLs you should provide hierarchy of resources. In our case we have a collection of command*s* which can look like this: commands/ |-- configuration |-- connections |-- connection_stat_reset |-- dump |-- environment |-- get_trace_mask `-- monitor Then, to get the environment one must issue a GET request to the URL /v1/api/commands/environment .
          Hide
          Skye Wanderman-Milne added a comment -

          Nikita, thank you for your comment. I've posted a new patch on review board (https://reviews.apache.org/r/8094/, will post it here shortly) which makes the server root and command URL configurable, so I don't think the /v1 prefix is necessary – given that it's unlikely we'll create a new API anytime soon and that users can change the URLs to whatever they please, I think it's best to use a simple default URL rather than the type you'd use for an actual web service. I don't feel particularly strongly about this though so either works for me.

          I will definitely change the default URL from "command" to "commands" though (I keep accidentally typing "commands" so it must be the right choice ).

          After giving it more thought, I'm a little wary of making the interface too "RESTy", or at least being careful in how far we take the resource metaphor. Commands are more like RPCs than resources, both in function and implementation, and while we could force them into a CRUD-style GET/PUT/POST/DELETE interface I don't think it's a natural model. Each of the commands is different, so each would support a different set of operations with different semantics (e.g., DELETE "stats" to reset stats and DELETE "connections/xxx" to close a client connection – not implemented yet but conceivable). Also, we may add commands that aren't resource-oriented at all (e.g., a command to restart the ZK instance).

          I also like using GETs whenever possible because they're dead simple (you can link to them!). It's certainly a very minor inconvenience to use other request types, but in this case I don't think it's worth it given that it doesn't add clarity to the API.

          Show
          Skye Wanderman-Milne added a comment - Nikita, thank you for your comment. I've posted a new patch on review board ( https://reviews.apache.org/r/8094/ , will post it here shortly) which makes the server root and command URL configurable, so I don't think the /v1 prefix is necessary – given that it's unlikely we'll create a new API anytime soon and that users can change the URLs to whatever they please, I think it's best to use a simple default URL rather than the type you'd use for an actual web service. I don't feel particularly strongly about this though so either works for me. I will definitely change the default URL from "command" to "commands" though (I keep accidentally typing "commands" so it must be the right choice ). After giving it more thought, I'm a little wary of making the interface too "RESTy", or at least being careful in how far we take the resource metaphor. Commands are more like RPCs than resources, both in function and implementation, and while we could force them into a CRUD-style GET/PUT/POST/DELETE interface I don't think it's a natural model. Each of the commands is different, so each would support a different set of operations with different semantics (e.g., DELETE "stats" to reset stats and DELETE "connections/xxx" to close a client connection – not implemented yet but conceivable). Also, we may add commands that aren't resource-oriented at all (e.g., a command to restart the ZK instance). I also like using GETs whenever possible because they're dead simple (you can link to them!). It's certainly a very minor inconvenience to use other request types, but in this case I don't think it's worth it given that it doesn't add clarity to the API.
          Hide
          Skye Wanderman-Milne added a comment -

          New patch that includes many minor fixes (renaming, adding comments, etc.). The biggest change is that the AdminServer is started before a ZK server is created (this mimics the behavior of the current 4lw commands – the CnxnFactory is created before the ZK server), and now an AdminServer is started for quorum members and not just standalone servers.

          Show
          Skye Wanderman-Milne added a comment - New patch that includes many minor fixes (renaming, adding comments, etc.). The biggest change is that the AdminServer is started before a ZK server is created (this mimics the behavior of the current 4lw commands – the CnxnFactory is created before the ZK server), and now an AdminServer is started for quorum members and not just standalone servers.
          Hide
          Nikita Vetoshkin added a comment -

          Skye, yes, I think current amount of ReST is quite enough to start with

          Show
          Nikita Vetoshkin added a comment - Skye, yes, I think current amount of ReST is quite enough to start with
          Hide
          Ted Dunning added a comment -

          Regarding resty-ness, I think that this interface is a real candidate for not using the GET/PUT/POST/DELETE verb at all but simply post-pending a verb to the URL of appropriate report. For that matter, the available verbs could be added to each report for convenience if there is an HTML view.

          This allows <mumble>/<connection>/close which is much more clearly self-documenting than a guess about what different operations might do to the connection in question.

          Show
          Ted Dunning added a comment - Regarding resty-ness, I think that this interface is a real candidate for not using the GET/PUT/POST/DELETE verb at all but simply post-pending a verb to the URL of appropriate report. For that matter, the available verbs could be added to each report for convenience if there is an HTML view. This allows <mumble>/<connection>/close which is much more clearly self-documenting than a guess about what different operations might do to the connection in question.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12555882/ZOOKEEPER-1346.2.patch
          against trunk revision 1422772.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1305//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/12555882/ZOOKEEPER-1346.2.patch against trunk revision 1422772. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1305//console This message is automatically generated.
          Hide
          Skye Wanderman-Milne added a comment -

          Oops, haven't rebased with trunk in a while, will fix in next patch

          Show
          Skye Wanderman-Milne added a comment - Oops, haven't rebased with trunk in a while, will fix in next patch
          Hide
          Skye Wanderman-Milne added a comment -

          Added CommandsTest, documentation to the ZooKeeper Admin's Guide, rebased on trunk, small fixes/improvements from comments.

          Show
          Skye Wanderman-Milne added a comment - Added CommandsTest, documentation to the ZooKeeper Admin's Guide, rebased on trunk, small fixes/improvements from comments.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12561447/ZOOKEEPER-1346.3.patch
          against trunk revision 1423996.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1314//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/12561447/ZOOKEEPER-1346.3.patch against trunk revision 1423996. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1314//console This message is automatically generated.
          Hide
          Camille Fournier added a comment -

          Can you try again with a patch that successfully applies to trunk?

          Show
          Camille Fournier added a comment - Can you try again with a patch that successfully applies to trunk?
          Hide
          Skye Wanderman-Milne added a comment -

          Rebased on trunk again (Unfortunately this required some pretty large changes, making the diff between this patch and the previous one hard to read.)

          Made it possible to disable the AdminServer either by setting the zookeeper.admin.enableAdminServer system property to false or by removing jetty from the classpath (in case users don't want to depend on Jetty). I implemented this by extracting AdminServer into an interface with two subclasses, JettyAdminServer (the original implementation) and DummyAdminServer (which does nothing and is used when the server is disabled). AdminServerFactory is then responsible for creating the appropriate server. I updated the documentation to reflect this.

          Using the system property, I disabled the AdminServer during tests as it was causing some tests to hang when they tried to start multiple AdminServers on the same port. I also added some more comments and renamed some functions to make them clearer.

          Show
          Skye Wanderman-Milne added a comment - Rebased on trunk again (Unfortunately this required some pretty large changes, making the diff between this patch and the previous one hard to read.) Made it possible to disable the AdminServer either by setting the zookeeper.admin.enableAdminServer system property to false or by removing jetty from the classpath (in case users don't want to depend on Jetty). I implemented this by extracting AdminServer into an interface with two subclasses, JettyAdminServer (the original implementation) and DummyAdminServer (which does nothing and is used when the server is disabled). AdminServerFactory is then responsible for creating the appropriate server. I updated the documentation to reflect this. Using the system property, I disabled the AdminServer during tests as it was causing some tests to hang when they tried to start multiple AdminServers on the same port. I also added some more comments and renamed some functions to make them clearer.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563216/ZOOKEEPER-1346.4.patch
          against trunk revision 1427034.

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

          +1 tests included. The patch appears to include 11 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 appears to introduce 2 new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//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/12563216/ZOOKEEPER-1346.4.patch against trunk revision 1427034. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1320//console This message is automatically generated.
          Hide
          Edward Ribeiro added a comment -

          The patch ZOOKEEPER-1346.4.patch reintroduced some lines that I removed from src/java/main/org/apache/zookeeper/ZooKeeperMain.java to fix ZOOKEEPER-1535.

          Did you find any error in that fix?

          Show
          Edward Ribeiro added a comment - The patch ZOOKEEPER-1346 .4.patch reintroduced some lines that I removed from src/java/main/org/apache/zookeeper/ZooKeeperMain.java to fix ZOOKEEPER-1535 . Did you find any error in that fix?
          Hide
          Edward Ribeiro added a comment -

          I am sending a patch that preserves ZOOKEEPER-1535 and fixes the FindBugs warnings (hopefully, both). See if it's okay.

          Show
          Edward Ribeiro added a comment - I am sending a patch that preserves ZOOKEEPER-1535 and fixes the FindBugs warnings (hopefully, both). See if it's okay.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563234/ZOOKEEPER-1346.5.patch
          against trunk revision 1427034.

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

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

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

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +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/1321//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1321//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/12563234/ZOOKEEPER-1346.5.patch against trunk revision 1427034. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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/1321//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1321//console This message is automatically generated.
          Hide
          Skye Wanderman-Milne added a comment -

          Oops, sorry about that Edward, I somehow lost that commit in the rebase. JIRA isn't letting me download your patch but I made a new one that adds ZOOKEEPER-1535 back and addresses one of the Findbugs warnings I introduced (I couldn't figure out how to fix the BC_UNCONFIRMED_CAST: Unchecked/unconfirmed cast warning).

          Show
          Skye Wanderman-Milne added a comment - Oops, sorry about that Edward, I somehow lost that commit in the rebase. JIRA isn't letting me download your patch but I made a new one that adds ZOOKEEPER-1535 back and addresses one of the Findbugs warnings I introduced (I couldn't figure out how to fix the BC_UNCONFIRMED_CAST: Unchecked/unconfirmed cast warning).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12563342/ZOOKEEPER-1346.6.patch
          against trunk revision 1427034.

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

          +1 tests included. The patch appears to include 11 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 appears to introduce 1 new Findbugs (version 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//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/12563342/ZOOKEEPER-1346.6.patch against trunk revision 1427034. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 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 appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1322//console This message is automatically generated.
          Hide
          Edward Ribeiro added a comment -

          No problem, man.

          Well, as for the 2nd Findbugs message, I'd suggest to modify the code at Commands.MonitorCommand.run() as below:
          [code]
          if (stats.getServerState().equals("leader") && zkServer instanceof LeaderZooKeeperServer) {
          Leader leader = ((LeaderZooKeeperServer) zkServer).getLeader();
          [/code]

          It worked on my tests with Findbugs here. //Type checking can set up many traps.

          Cheers,
          Ed

          Show
          Edward Ribeiro added a comment - No problem, man. Well, as for the 2nd Findbugs message, I'd suggest to modify the code at Commands.MonitorCommand.run() as below: [code] if (stats.getServerState().equals("leader") && zkServer instanceof LeaderZooKeeperServer) { Leader leader = ((LeaderZooKeeperServer) zkServer).getLeader(); [/code] It worked on my tests with Findbugs here. //Type checking can set up many traps. Cheers, Ed
          Hide
          Skye Wanderman-Milne added a comment -

          Added JettyAdminServerTest. This required some changes to make it work such as shutting down the Jetty server to make sure it released it port between tests and changing where the admin system properties are read so they are refreshed when a new server is created.

          Rebased on trunk and addressed review comments.

          Show
          Skye Wanderman-Milne added a comment - Added JettyAdminServerTest. This required some changes to make it work such as shutting down the Jetty server to make sure it released it port between tests and changing where the admin system properties are read so they are refreshed when a new server is created. Rebased on trunk and addressed review comments.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12565347/ZOOKEEPER-1346.7.patch
          against trunk revision 1433651.

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

          +1 tests included. The patch appears to include 19 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 1.3.9) warnings.

          -1 release audit. The applied patch generated 25 release audit warnings (more than the trunk's current 24 warnings).

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//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/12565347/ZOOKEEPER-1346.7.patch against trunk revision 1433651. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 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 1.3.9) warnings. -1 release audit. The applied patch generated 25 release audit warnings (more than the trunk's current 24 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1341//console This message is automatically generated.
          Hide
          Edward Ribeiro added a comment -

          I didn't get why those – seemingly unrelated to the patch – are being thrown. Skye only have to put a License header in org/apache/zookeeper/server/admin/JettyAdminServerTest.java. But the patch looks okay to me.

          Show
          Edward Ribeiro added a comment - I didn't get why those – seemingly unrelated to the patch – are being thrown. Skye only have to put a License header in org/apache/zookeeper/server/admin/JettyAdminServerTest.java. But the patch looks okay to me.
          Hide
          Skye Wanderman-Milne added a comment -

          Added license to JettyAdminServerTest.

          Show
          Skye Wanderman-Milne added a comment - Added license to JettyAdminServerTest.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12565737/ZOOKEEPER-1346.8.patch
          against trunk revision 1434978.

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

          +1 tests included. The patch appears to include 19 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 1.3.9) warnings.

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

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//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/12565737/ZOOKEEPER-1346.8.patch against trunk revision 1434978. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 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 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1348//console This message is automatically generated.
          Hide
          Edward Ribeiro added a comment -

          Very nice. Congratulations, Skye.

          Do you mind to close the two review issues I opened at https://reviews.apache.org/r/8094/ as done, please? Just mark them as checked, because your patch is ready to ship, imo.

          Show
          Edward Ribeiro added a comment - Very nice. Congratulations, Skye. Do you mind to close the two review issues I opened at https://reviews.apache.org/r/8094/ as done, please? Just mark them as checked, because your patch is ready to ship, imo.
          Hide
          Edward Ribeiro added a comment -

          Hi,

          It just occurred me that there are some classes that the current version of this patch doesn't contemplate:

          /src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java
          /src/java/test/org/apache/zookeeper/test/FourLetterWordsQuorumTest.java
          /src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java

          It would be nice to take a look at least on the first one. Did you already have anything planned to them? I really don't know if it's better to include it in another version of your patch, create a separate patch to address them, or just deprecate them all. What do you think? Or I misunderstood something.

          PS: The FourLetterWordMain rewrite seems just a matter of making a URLConnection and read the input, no big deal really.

          Cheers,
          E.

          Show
          Edward Ribeiro added a comment - Hi, It just occurred me that there are some classes that the current version of this patch doesn't contemplate: /src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java /src/java/test/org/apache/zookeeper/test/FourLetterWordsQuorumTest.java /src/java/test/org/apache/zookeeper/test/FourLetterWordsTest.java It would be nice to take a look at least on the first one. Did you already have anything planned to them? I really don't know if it's better to include it in another version of your patch, create a separate patch to address them, or just deprecate them all. What do you think? Or I misunderstood something. PS: The FourLetterWordMain rewrite seems just a matter of making a URLConnection and read the input, no big deal really. Cheers, E.
          Hide
          Edward Ribeiro added a comment -

          On a related issue, I have written a patch to address ZOOKEEPER-1423 – that is directly affected by this --, but count on me to redo it as soon as this patch gets committed to trunk.

          Show
          Edward Ribeiro added a comment - On a related issue, I have written a patch to address ZOOKEEPER-1423 – that is directly affected by this --, but count on me to redo it as soon as this patch gets committed to trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12565737/ZOOKEEPER-1346.8.patch
          against trunk revision 1528271.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1619//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/12565737/ZOOKEEPER-1346.8.patch against trunk revision 1528271. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1619//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/12565737/ZOOKEEPER-1346.8.patch
          against trunk revision 1586200.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2032//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/12565737/ZOOKEEPER-1346.8.patch against trunk revision 1586200. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/2032//console This message is automatically generated.
          Hide
          Bill Havanki added a comment -

          Hi all,

          After syncing up with Skye, I rebased the latest patch here (#8) against trunk. Since it's been well over a year since there was activity here, and the rebase wasn't clean, I'd like to submit the changes for fresh review.

          Does anyone have a preference for whether I submit it all as a single patch here, or break it up into several smaller patches for easier consumption? Personally, I prefer more, smaller patches, and I'd be happy to post them under subtasks to this JIRA for tracking purposes.

          I can also use Review Board (reviews.apache.org) for posting and soliciting feedback, if the project uses that.

          Thanks!
          Bill

          Show
          Bill Havanki added a comment - Hi all, After syncing up with Skye, I rebased the latest patch here (#8) against trunk. Since it's been well over a year since there was activity here, and the rebase wasn't clean, I'd like to submit the changes for fresh review. Does anyone have a preference for whether I submit it all as a single patch here, or break it up into several smaller patches for easier consumption? Personally, I prefer more, smaller patches, and I'd be happy to post them under subtasks to this JIRA for tracking purposes. I can also use Review Board (reviews.apache.org) for posting and soliciting feedback, if the project uses that. Thanks! Bill
          Hide
          Raul Gutierrez Segales added a comment -

          Hi Bill Havanki,

          having this patch would be awesome. fwiw, i think reviewboards (even if long) are easier to review than attached patches. If we used Github then a PR with nice small commits would have been even better but we don't, alas .

          Show
          Raul Gutierrez Segales added a comment - Hi Bill Havanki , having this patch would be awesome. fwiw, i think reviewboards (even if long) are easier to review than attached patches. If we used Github then a PR with nice small commits would have been even better but we don't, alas .
          Hide
          Patrick Hunt added a comment -

          Perhaps finish this one out with a big patch via RB, then do subsequent smaller ones?

          Show
          Patrick Hunt added a comment - Perhaps finish this one out with a big patch via RB, then do subsequent smaller ones?
          Hide
          Bill Havanki added a comment -

          Thanks for the feedback. Review posted on RB!

          https://reviews.apache.org/r/23259/

          Show
          Bill Havanki added a comment - Thanks for the feedback. Review posted on RB! https://reviews.apache.org/r/23259/
          Hide
          Flavio Junqueira added a comment -

          Is the fix version correctly set to 3.6.0? Sounds like a good feature to have.

          Show
          Flavio Junqueira added a comment - Is the fix version correctly set to 3.6.0? Sounds like a good feature to have.
          Hide
          Patrick Hunt added a comment -

          Agree, would be great to get into 3.5.x.

          Show
          Patrick Hunt added a comment - Agree, would be great to get into 3.5.x.
          Hide
          Raul Gutierrez Segales added a comment -

          Added some comments to the RB, would be awesome to have this indeed.

          Show
          Raul Gutierrez Segales added a comment - Added some comments to the RB, would be awesome to have this indeed.
          Hide
          Raul Gutierrez Segales added a comment -

          Added some more comments on the reviewboard, mainly questions about diverging impls in the Netty vs NIO ServerCnxnFactory and smaller nits. Oh, and one more case in which I think a well defined defined class is better than Map<> for returning values.

          After that I am happy to +1 this. Thanks Bill Havanki!

          Show
          Raul Gutierrez Segales added a comment - Added some more comments on the reviewboard, mainly questions about diverging impls in the Netty vs NIO ServerCnxnFactory and smaller nits. Oh, and one more case in which I think a well defined defined class is better than Map<> for returning values. After that I am happy to +1 this. Thanks Bill Havanki !

            People

            • Assignee:
              Bill Havanki
              Reporter:
              Camille Fournier
            • Votes:
              1 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:

                Development