> On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/service/XLogService.java, line > > 194 > > <https://reviews.apache.org/r/11922/diff/5/?file=323876#file323876line194> > > > > why this var? from looking at INIT this is always set to true
I think it was mostly for some tests, but if originally the log4j configuration was such that streaming should be disabled, but was later changed such that streaming should be enabled, and then extractInfoForLogWebService(...) is called again (to let Oozie know about the change), then without setting logOverWS back to true here, it will stay false (because this method only ever sets it to false). > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java, > > line 97 > > <https://reviews.apache.org/r/11922/diff/5/?file=323878#file323878line97> > > > > if firstId is NULL that means this server is the first one? how come? That was left over from before; firstId can't actually be null there anymore so I'll remove that > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java, > > line 102 > > <https://reviews.apache.org/r/11922/diff/5/?file=323878#file323878line102> > > > > 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) > > The reason I didn't do that was to minimize the number of calls to ZooKeeper. Calling zk.getAllMetadata() gives us the list of Oozie servers' metadata; this is used to determine both the number of oozie servers and this server's index. So, getOozieServerIndex() would have to return two values (the index of this server and the number of servers). I thought it was better to just duplicate the 3 lines: List<ServiceInstance<Map>> oozies = zk.getAllMetaData(); int numOozies = oozies.size(); int myIndex = zk.getZKIdIndex(oozies); However, I can make a private method for checking if the job id belongs to this server to remove some of the duplication, so I'll do that. > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/service/ZKLocksService.java, > > line 115 > > <https://reviews.apache.org/r/11922/diff/5/?file=323879#file323879line115> > > > > do we want to lock forever with -1 in the case of ZK or we should have > > an arbitrary HIGH MAX wait? That's an interesting point; the original MemoryLocks has this too and it hasn't been a problem. That said, I don't think we're every passing a wait of -1 anywhere though. > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/util/ConfigUtils.java, line 70 > > <https://reviews.apache.org/r/11922/diff/5/?file=323888#file323888line70> > > > > 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? > > I've combined getOozieHttpUrl() and getOozieHttpsUrl() into one method as you suggested (getOozieUrl(boolean secure)) and also used StringBuilder. The getOozieEffectiveUrl() method (which will now call the getOozieUrl(...) method) is to get the address of whichever configuration is actually being used. It's more convenient and easier to find than looking at ServicesLoader.isSSLEnabled(). > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/resources/oozie-default.xml, line 1951 > > <https://reviews.apache.org/r/11922/diff/5/?file=323894#file323894line1951> > > > > what happens if somebody sets it to emtpy <value></value>? It should > > work just fine, rigth? I thought Configuration doesn't properly handle empty values? There's a number of comments in oozie-default to that effect and all other "blank" values have a space instead of being empty. > On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote: > > /trunk/core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java, > > line 106 > > <https://reviews.apache.org/r/11922/diff/5/?file=323880#file323880line106> > > > > 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 > > On Aug. 1, 2013, 7:03 p.m., Robert Kanter wrote: > > 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. Most of the public methods are getters for non-changing values or are querying ZK, so those shouldn't be a problem. The register(...) and unregister(...) methods can have a race condition if multiple of the ZK___Service classes try to call them at the same time, so I made those synchronized (though I don't think this normally happens anyway). Some of the private methods would be a problem, except that they are all called from either register(...) or unregister(...) so that shouldn't be an issue. Just shows you how modular Oozie is :) - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11922/#review24459 ----------------------------------------------------------- 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 > >
