> On April 17, 2017, 11:42 p.m., Satish Saley wrote: > > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java > > Lines 221 (patched) > > <https://reviews.apache.org/r/58406/diff/1/?file=1692867#file1692867line249> > > > > Please move this inside synchronized block on line below to get > > consistent view of slaCalc object across threads. Adjust the code inside > > checkAndUpdateSLACalcAfterRestart-->updateSLARegistartion accordingly. We > > don't need the synchronized block in updateSLARegistartion then.
We need to synchronize block so that only one thread update the slaCalc. > On April 17, 2017, 11:42 p.m., Satish Saley wrote: > > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java > > Line 250 (original), 234 (patched) > > <https://reviews.apache.org/r/58406/diff/1/?file=1692867#file1692867line262> > > > > Do we need else condition similar to that line 265? > > > > ``` > > else{ > > if (firstCheckAfterRetstart) { > > slaCalc.setSLARegistrationBean(null); > > } > > } > > ``` Not sure what do you mean by this? Are you saying that we need to check following condition? if (e.getErrorCode().equals(ErrorCode.E0604) || e.getErrorCode().equals(ErrorCode.E0605)) { LOG.debug("job [{0}] is is not in DB, removing from Memory", slaCalc.getId()); slaMap.remove(jobId); } If yes, No we don't have to check if an object exists or not. addJobStatus will get called when there is a change in job status which means the object is in DB. > On April 17, 2017, 11:42 p.m., Satish Saley wrote: > > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java > > Line 476 (original), 464 (patched) > > <https://reviews.apache.org/r/58406/diff/1/?file=1692867#file1692867line493> > > > > Need to wrap the call to checkAndUpdateSLACalcAfterRestart in > > synchronized(slaCalc) {} with the changes I suggested regarding > > synchronized block on line 221. If I understand correctly, Do you mean to say that we should synchronize checkAndUpdateSLACalcAfterRestart. We don't want to synchronized check. Only update need synchronization. > On April 17, 2017, 11:42 p.m., Satish Saley wrote: > > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java > > Lines 657 (patched) > > <https://reviews.apache.org/r/58406/diff/1/?file=1692867#file1692867line686> > > > > No need to check slaCalc.getSLARegistrationBean() == null here, inside > > synchronized(slaCalc) we are doing it. Yes. If the call fails, we need to make sure that next run will execute SLA check. - Purshotam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58406/#review172149 ----------------------------------------------------------- On April 14, 2017, 10:41 p.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58406/ > ----------------------------------------------------------- > > (Updated April 14, 2017, 10:41 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2863 > https://issues.apache.org/jira/browse/OOZIE-2863 > > > Repository: oozie-git > > > Description > ------- > > SLACalculatorMemory.loadOnRestart causing delay in server start > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java 3a76dfe6 > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 347f8537 > core/src/test/java/org/apache/oozie/service/TestHASLAService.java 3af263e1 > core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java > 559e2b31 > > > Diff: https://reviews.apache.org/r/58406/diff/1/ > > > Testing > ------- > > > Thanks, > > Purshotam Shah > >
