----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17720/#review42969 -----------------------------------------------------------
Doesn't handle case of removing from historySet. Similar to HCat HA we need a thread running once in a day (1 day is good enough as it is going to be occupying very less memory) and removing entries from historySet if job is already completed. core/src/main/java/org/apache/oozie/executor/jpa/SLARegistrationQueryExecutor.java <https://reviews.apache.org/r/17720/#comment76940> Delete SLARegistrationGetJPAExecutor and SLARegistrationGetonRestartJPAExecutor core/src/main/java/org/apache/oozie/executor/jpa/SLASummaryQueryExecutor.java <https://reviews.apache.org/r/17720/#comment76941> Can we call this method getSingleValue() and add it to QueryExecutor (implementation that throws UnSupportedException instead of abstract) and override here. There are some jpaexecutors (For eg: count of CoordinatorActionBean) which return count and this method can be used in common for that. core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java <https://reviews.apache.org/r/17720/#comment76944> As mentioned in earlier comment lock is not required for non HA mode. It will introduce unnecessary latency. If you want to keep code simple return a static DummyLockToken object which has empty implementation for release() if (!JobsConcurrencyService. multipleServerRunning()) { lock = DUMMY_LOCKTOKEN; } core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java <https://reviews.apache.org/r/17720/#comment76942> Empty line between methods core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java <https://reviews.apache.org/r/17720/#comment76945> Shouldn't we be calling slaCalcStatus.acquireLock() instead of calling directly? core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java <https://reviews.apache.org/r/17720/#comment76943> Do not have to catch exception at all and throw them again. Remove the catch blocks. core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java <https://reviews.apache.org/r/17720/#comment76947> Keep this logic in SLACalcStatus core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java <https://reviews.apache.org/r/17720/#comment76948> Why queue event when there is no expected start? Accidental change? core/src/main/java/org/apache/oozie/sla/SLARegistrationBean.java <https://reviews.apache.org/r/17720/#comment76946> When is apptype null? core/src/test/java/org/apache/oozie/service/TestHASLAService.java <https://reviews.apache.org/r/17720/#comment76950> We need to have test cases where two instances are started with lot of entries loaded on start and actions performed alternatively on one another and check if other reflects the change on updateAllSlaStatus() to actually test code path while both servers are running. core/src/test/java/org/apache/oozie/service/TestHASLAService.java <https://reviews.apache.org/r/17720/#comment76949> Test is redundant and can be removed as load on restart has nothing changed between HA and normal case and does not test anything new. core/src/test/java/org/apache/oozie/service/TestHASLAService.java <https://reviews.apache.org/r/17720/#comment76951> This test also does not exercise HA code path without ZKJobsConcurrencyService. This test is just equivalent to having one server in non-HA mode restarted. - Rohini Palaniswamy On May 14, 2014, 2:26 a.m., Ryota Egashira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17720/ > ----------------------------------------------------------- > > (Updated May 14, 2014, 2:26 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1678 > https://issues.apache.org/jira/browse/OOZIE-1678 > > > Repository: oozie-git > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-1678 > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/command/coord/CoordChangeXCommand.java > fb31e9a > > core/src/main/java/org/apache/oozie/executor/jpa/SLARegistrationQueryExecutor.java > e3b115f > > core/src/main/java/org/apache/oozie/executor/jpa/SLASummaryQueryExecutor.java > 79d11ed > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetJPAExecutor.java > 1f7cb4d > core/src/main/java/org/apache/oozie/service/JPAService.java aba8709 > core/src/main/java/org/apache/oozie/service/JobsConcurrencyService.java > 27c97e6 > core/src/main/java/org/apache/oozie/service/ZKJobsConcurrencyService.java > 611b74c > core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java ea53712 > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 618d899 > core/src/main/java/org/apache/oozie/sla/SLARegistrationBean.java a2260a4 > core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java 0a70326 > core/src/main/java/org/apache/oozie/sla/service/SLAService.java ea2983f > > core/src/test/java/org/apache/oozie/command/coord/TestCoordChangeXCommand.java > 327ec90 > > core/src/test/java/org/apache/oozie/executor/jpa/TestSLARegistrationQueryExecutor.java > 00fb677 > > core/src/test/java/org/apache/oozie/executor/jpa/TestSLASummaryQueryExecutor.java > 2e170a4 > core/src/test/java/org/apache/oozie/service/TestHASLAService.java > PRE-CREATION > core/src/test/java/org/apache/oozie/sla/TestSLACalculationJPAExecutor.java > 9270aa2 > core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java > 9a16722 > core/src/test/java/org/apache/oozie/sla/TestSLAEventGeneration.java f3bfc29 > core/src/test/java/org/apache/oozie/sla/TestSLAJobEventListener.java > 3ce86ab > core/src/test/java/org/apache/oozie/sla/TestSLAService.java 291d850 > > core/src/test/java/org/apache/oozie/sla/TestSLASummaryGetOnRestartJPAExecutor.java > 3a9e72e > core/src/test/java/org/apache/oozie/test/ZKXTestCase.java 7bebaf0 > core/src/test/resources/coord-action-sla.xml e88df6c > > Diff: https://reviews.apache.org/r/17720/diff/ > > > Testing > ------- > > > Thanks, > > Ryota Egashira > >
