----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16756/#review32335 -----------------------------------------------------------
Overall, great job. I tested this out on a cluster and its working properly. Just some minor things... core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java <https://reviews.apache.org/r/16756/#comment61080> Can you also put the comments that were originally above the URL? i.e. // It's important that we specify ALL_SERVERS_PARAM=false in the GET request to prevent the other Oozie Server from trying // aggregate logs from the other Oozie servers (and creating an infinite recursion) core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java <https://reviews.apache.org/r/16756/#comment61081> I think we should switch the logic around to be consistent with the log streaming. i.e. if(... == null || ... == "true") This way, it will default to doing it for all servers unless the parameter is explicitly set to false. core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java <https://reviews.apache.org/r/16756/#comment61082> Also add comments about how =false is important core/src/main/java/org/apache/oozie/util/AuthUrlClient.java <https://reviews.apache.org/r/16756/#comment61083> Moving these things into their own class was a good idea and is cleaner than what I had originally :) But I think we should make this a static class or a singleton. The authenticator class type shouldn't change without restarting Oozie, so once we call determineAuthenticatorClassType() it shouldn't change; this class also has no other state, so its methods can all be made static so callers don't have to instantiate an AuthUrlClient object to use these them. core/src/test/java/org/apache/oozie/service/DummyV2AdminServlet.java <https://reviews.apache.org/r/16756/#comment61084> Shouldn't this go in the DummyV2AdminServlet constructor? And adminServlet be a class variable? And shouldn't it be a V2AdminServlet? (V2 subclasses V1) core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java <https://reviews.apache.org/r/16756/#comment61085> Shouldn't this be V2? core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java <https://reviews.apache.org/r/16756/#comment61086> Shouldn't this be V2? core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java <https://reviews.apache.org/r/16756/#comment61087> Shouldn't this be V2? - Robert Kanter On Jan. 9, 2014, 10:45 p.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16756/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2014, 10:45 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1609 > https://issues.apache.org/jira/browse/OOZIE-1609 > > > Repository: oozie-git > > > Description > ------- > > Sharelib support for HA. > > 1.Sharelib update : Server calls other server to update sharelib. > 2.Purging of sharelib : only first server deletes sharelib. > > > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/OozieClient.java 9d6c9e0 > core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java > 99c16e0 > core/src/main/java/org/apache/oozie/service/ShareLibService.java 9556620 > core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java > 42fce05 > core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java > c17a8aa > core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java 091070f > core/src/main/java/org/apache/oozie/util/AuthUrlClient.java e69de29 > core/src/test/java/org/apache/oozie/service/DummyV2AdminServlet.java > e69de29 > core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java > e69de29 > docs/src/site/twiki/DG_CommandLineTool.twiki af472d3 > docs/src/site/twiki/WebServicesAPI.twiki 50795b4 > > Diff: https://reviews.apache.org/r/16756/diff/ > > > Testing > ------- > > > Thanks, > > Purshotam Shah > >
