----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11922/#review24459 -----------------------------------------------------------
/trunk/core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48379> it is not really dummy, but used for non-HA, update javadoc in that regard /trunk/core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48381> method name is a bit odd, how about 'isJobIdForThisServer' /trunk/core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48380> method name is a bit odd, how about 'getJobIdsForThisServer' /trunk/core/src/main/java/org/apache/oozie/service/MemoryLocksService.java <https://reviews.apache.org/r/11922/#comment48382> unused imports these 2 /trunk/core/src/main/java/org/apache/oozie/service/XLogService.java <https://reviews.apache.org/r/11922/#comment48389> why this var? from looking at INIT this is always set to true /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48383> if firstId is NULL that means this server is the first one? how come? /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48384> the following 2 methods have duplicated logic, we should have 2 private methods: getOozieServerIndex() doing the zk.getAllMetadata()...zk.getZKIdIndex() logic isJobIdForThisServer(int serverIndex, String jobId) /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48385> here we call isJobIdForThisServer(getOozieServerIndex(), jobId) /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48386> here we call serverIndex = getOozieServerIndex(); /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java <https://reviews.apache.org/r/11922/#comment48388> here we do if (isJobIdForThisServer(index, id)) /trunk/core/src/main/java/org/apache/oozie/service/ZKLocksService.java <https://reviews.apache.org/r/11922/#comment48390> do we want to lock forever with -1 in the case of ZK or we should have an arbitrary HIGH MAX wait? /trunk/core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java <https://reviews.apache.org/r/11922/#comment48391> we are caching all logs in memory before merging them, that is not good as all logs are already sorted, we can do a mergesort consuming from the different streams /trunk/core/src/main/java/org/apache/oozie/util/ConfigUtils.java <https://reviews.apache.org/r/11922/#comment48392> why not just a single getOozieUrl(boolean secure) and based on that you set http or https as schema? do we need 3 methods public or just the last one would be OK? /trunk/core/src/main/java/org/apache/oozie/util/FixedJsonInstanceSerializer.java <https://reviews.apache.org/r/11922/#comment48393> create OOZIE JIRA blocked by CURATOR-5 to remove this /trunk/core/src/main/resources/oozie-default.xml <https://reviews.apache.org/r/11922/#comment48395> what happens if somebody sets it to emtpy <value></value>? It should work just fine, rigth? /trunk/docs/src/site/twiki/AG_Install.twiki <https://reviews.apache.org/r/11922/#comment48396> no need to mention 'As with the database' /trunk/docs/src/site/twiki/AG_Install.twiki <https://reviews.apache.org/r/11922/#comment48397> add 'make sure you DON'T start them' /trunk/docs/src/site/twiki/AG_Install.twiki <https://reviews.apache.org/r/11922/#comment48398> clarify that to have real HA there should be at least 3 ZK servers may have a section stating what is required for real HA from Oozie, DB, network, loadbalancer, zookeeper /trunk/docs/src/site/twiki/AG_Install.twiki <https://reviews.apache.org/r/11922/#comment48399> can we use lowercase or this is a zookeeper name convention, mention that if there are 2 different oozie setups doing HA they should have different namespaces /trunk/docs/src/site/twiki/DG_CommandLineTool.twiki <https://reviews.apache.org/r/11922/#comment48400> add '(more than one only if HA is enabled)' (in the tool help too) Cannot comment directly on ZKUtils.java because it is not being loaded by RB. here is the comment for it: it seems methods in this class are not thread safe, shouldn't they be? Overall patch looks good, very nice that it takes a 200K patch (3000 lines of code including testcases) to add HA. - Alejandro Abdelnur On July 19, 2013, 11:58 p.m., Robert Kanter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11922/ > ----------------------------------------------------------- > > (Updated July 19, 2013, 11:58 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-615 > https://issues.apache.org/jira/browse/OOZIE-615 > > > Repository: oozie > > > Description > ------- > > See this comment for details: > https://issues.apache.org/jira/browse/OOZIE-615?focusedCommentId=13686181&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13686181 > > > Diffs > ----- > > /trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java 1505066 > /trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java > 1505066 > /trunk/client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 1505066 > /trunk/core/pom.xml 1505066 > /trunk/core/src/main/java/org/apache/oozie/BaseEngine.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/BundleEngine.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/DagEngine.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/command/Command.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1505066 > > /trunk/core/src/main/java/org/apache/oozie/service/ActionCheckerService.java > 1505066 > > /trunk/core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java > PRE-CREATION > /trunk/core/src/main/java/org/apache/oozie/service/MemoryLocksService.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/service/PauseTransitService.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/service/PurgeService.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/service/RecoveryService.java > 1505066 > > /trunk/core/src/main/java/org/apache/oozie/service/StatusTransitService.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/service/XLogService.java 1505066 > > /trunk/core/src/main/java/org/apache/oozie/service/XLogStreamingService.java > PRE-CREATION > > /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java > PRE-CREATION > /trunk/core/src/main/java/org/apache/oozie/service/ZKLocksService.java > PRE-CREATION > > /trunk/core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java > PRE-CREATION > /trunk/core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/ServicesLoader.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/V0AdminServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/V1AdminServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/servlet/V2AdminServlet.java > 1505066 > /trunk/core/src/main/java/org/apache/oozie/util/ConfigUtils.java 1505066 > > /trunk/core/src/main/java/org/apache/oozie/util/FixedJsonInstanceSerializer.java > PRE-CREATION > /trunk/core/src/main/java/org/apache/oozie/util/LockToken.java PRE-CREATION > /trunk/core/src/main/java/org/apache/oozie/util/MemoryLocks.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java 1505066 > /trunk/core/src/main/java/org/apache/oozie/util/ZKUtils.java PRE-CREATION > /trunk/core/src/main/resources/oozie-default.xml 1505066 > > /trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngineStreamLog.java > 1505066 > /trunk/core/src/test/java/org/apache/oozie/command/TestXCommand.java > 1505066 > > /trunk/core/src/test/java/org/apache/oozie/service/DummyLogStreamingServlet.java > PRE-CREATION > > /trunk/core/src/test/java/org/apache/oozie/service/TestJobsConcurrencyService.java > PRE-CREATION > /trunk/core/src/test/java/org/apache/oozie/service/TestXLogService.java > 1505066 > > /trunk/core/src/test/java/org/apache/oozie/service/TestXLogStreamingService.java > PRE-CREATION > > /trunk/core/src/test/java/org/apache/oozie/service/TestZKJobsConcurrencyService.java > PRE-CREATION > /trunk/core/src/test/java/org/apache/oozie/service/TestZKLocksService.java > PRE-CREATION > > /trunk/core/src/test/java/org/apache/oozie/service/TestZKXLogStreamingService.java > PRE-CREATION > > /trunk/core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java > 1505066 > > /trunk/core/src/test/java/org/apache/oozie/servlet/MockDagEngineService.java > 1505066 > /trunk/core/src/test/java/org/apache/oozie/test/ZKXTestCase.java > PRE-CREATION > /trunk/core/src/test/java/org/apache/oozie/util/TestMemoryLocks.java > 1505066 > /trunk/core/src/test/java/org/apache/oozie/util/TestXLogFilter.java 1505066 > /trunk/core/src/test/java/org/apache/oozie/util/TestZKUtils.java > PRE-CREATION > /trunk/distro/src/main/tomcat/ssl-web.xml 1505066 > /trunk/docs/src/site/twiki/AG_Install.twiki 1505066 > /trunk/docs/src/site/twiki/DG_CommandLineTool.twiki 1505066 > /trunk/docs/src/site/twiki/WebServicesAPI.twiki 1505066 > /trunk/pom.xml 1505066 > > Diff: https://reviews.apache.org/r/11922/diff/ > > > Testing > ------- > > > Thanks, > > Robert Kanter > >
