You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Alejandro Abdelnur <tu...@cloudera.com> on 2013/08/01 21:03:07 UTC
Re: Review Request 11922: OOZIE-615: Support high availability for the Oozie
service
-----------------------------------------------------------
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
>
>
Re: Review Request 11922: OOZIE-615: Support high availability for the Oozie
service
Posted by Robert Kanter <rk...@cloudera.com>.
> On Aug. 1, 2013, 7:03 p.m., Alejandro Abdelnur wrote:
> > /trunk/docs/src/site/twiki/AG_Install.twiki, line 762
> > <https://reviews.apache.org/r/11922/diff/5/?file=323911#file323911line762>
> >
> > 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
We can use lowercase
- 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
>
>
Re: Review Request 11922: OOZIE-615: Support high availability for the Oozie
service
Posted by Robert Kanter <rk...@cloudera.com>.
> 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
> >
>
> Robert Kanter wrote:
>
I'm not sure I exactly did a mergesort, but I did significantly change it to not load the entire logs into memory and merging them. It will now merge them as it reads them from the logs and write them to the output immediately.
- 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
>
>
Re: Review Request 11922: OOZIE-615: Support high availability for the Oozie
service
Posted by Robert Kanter <rk...@cloudera.com>.
> 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
>
>