Liran Zelkha has posted comments on this change. Change subject: core: Support transactions for Quartz jobs ......................................................................
Patch Set 7: (3 comments) http://gerrit.ovirt.org/#/c/36370/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/JobRepositoryCleanupManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/job/JobRepositoryCleanupManager.java: Line 50: * <li>The successful jobs will be deleted after {@code ConfigValues.SucceededJobCleanupTimeInMinutes}.</li> Line 51: * <li>The failed jobs will be deleted after {@code ConfigValues.FailedJobCleanupTimeInMinutes}.</li> Line 52: * <ul> Line 53: */ Line 54: @OnTimerMethodAnnotation(value = "completed_jobs_cleanup", transactional = true) > why this is required ? We need a transaction here, since each dao method needs to be in a transactional context for the JPA code to work. Line 55: public void cleanCompletedJob() { Line 56: Line 57: Date succeededJobsDeleteTime = Line 58: new Date(System.currentTimeMillis() - TimeUnit.MILLISECONDS.convert(succeededJobTime, TimeUnit.MINUTES)); http://gerrit.ovirt.org/#/c/36370/7/backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/JobWrapper.java File backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/JobWrapper.java: Line 42: final Object instance = paramsMap.get(SchedulerUtilQuartzImpl.RUNNABLE_INSTANCE); Line 43: final Object[] methodParams = (Object[]) paramsMap.get(SchedulerUtilQuartzImpl.RUN_METHOD_PARAM); Line 44: String methodKey = getMethodKey(instance.getClass().getName(), methodName); Line 45: final Method methodToRun; Line 46: if (!cachedMethods.containsKey(methodKey)) { > can't this synchronized block be replaced by ConcurrentHashMap.putIfAbsent I did it - but then changed my mind. It would be something like this: methodToRun = cachedMethods.putIfAbsent(methodKey, getMethodToRun(instance, methodName)); Meaning we always call the reflection based getMethodToRun - which is slow. I couldn't think of a better solution :-( Line 47: synchronized (cachedMethods) { Line 48: if (cachedMethods.containsKey(methodKey)) { Line 49: methodToRun = cachedMethods.get(methodKey); Line 50: } else { Line 67: @Override Line 68: public Exception runInTransaction() { Line 69: try { Line 70: methodToRun.invoke(instance, methodParams); Line 71: } catch (Exception e) { > why can't the return type be void ? If an exception is thrown by " methodTo True, but runInTransaction can't add a throws clause, so I can only rewrap the Exception as a RuntimeException - which I don't really like... Line 72: return e; Line 73: } Line 74: return null; Line 75: } -- To view, visit http://gerrit.ovirt.org/36370 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7eb67e6aeef9080b5647cb84b1cf701c720ce29d Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
