[data-access] Use the OSGi Transaction Control service to manage transactions #111
Conversation
| try { | ||
| HibernateUtil.getTransactionControl().required(() -> | ||
| new CompleteJobTransaction<DistributedAppliance>(DistributedAppliance.class) | ||
| .run(HibernateUtil.getTransactionalEntityManager(), new CompleteJobTransactionInput(mc.getId(), job.getId()))); |
| @@ -427,33 +435,37 @@ public void completed(Job job) { | |||
| } | |||
|
|
|||
| public static void updateSGJob(EntityManager em, final SecurityGroup sg, Job job) { | |||
|
|
|||
| //TODO it would be more sensible to inject the transactional entitymanager | |||
There was a problem hiding this comment.
Is this planned to happen next on a future Paremus PR?
There was a problem hiding this comment.
Yes this will happen as part of work item A7.
|
|
||
| // Assert | ||
| Assert.assertEquals("The received JobID in force delete case is different than expected.", JOB_ID, baseJobResponse.getJobId()); | ||
| Mockito.verify(this.tx, Mockito.times(0)).begin(); | ||
| Mockito.verify(this.tx, Mockito.times(1)).begin(); |
There was a problem hiding this comment.
I am assuming this does not represent any functional change, instead it is just a result of the refactoring. Is that correct? Same for line 203 and 204.
| } catch (Exception e) { | ||
|
|
||
| // TODO remove when EM and TX are injected |
There was a problem hiding this comment.
todo with owner please like// TODO:timwardx
| } catch (Exception e) { | ||
|
|
||
| // TODO remove when EM and TX are injected | ||
| log.error("Fail to update JobRecord " + this, e); |
There was a problem hiding this comment.
My understanding is that the EM should not be closed here because it is shared. See DBConnectionManager. It is created by the JPAEntityManagerProvider, which generates an EM that automatically associates with the current transaction scope.
There was a problem hiding this comment.
ahh...sorry missed the part about this being a TransactionalEntityManager
| this.jobRecord = em.find(JobRecord.class, this.jobRecord.getId()); | ||
| this.jobRecord.setStatus(getEntityStatus()); | ||
| OSCEntityManager.update(em, this.jobRecord); | ||
| return null; |
There was a problem hiding this comment.
broadcasts need to be done after tx is successful TransactionalBroadcastUtil.broadcast(em);
if tx is unsuccessful, broadcasts need to be removed TransactionalBroadcastUtil.removeSessionFromMap(em)
also catch all exceptions thrown by required method as was done in the previous code.
There was a problem hiding this comment.
Could you please clarify this comment. Under the current code, the broadcast is only sent after a successful transaction, as it is done in the post-completion hook of the TransactionContext. See TransactionBroadcastUtil.java, lines 74-78. If the transaction is unsuccessful then no broadcast is sent and the queued messages are released for GC.
| this.jobRecord = em.find(JobRecord.class, this.jobRecord.getId()); | ||
| this.jobRecord.setStatus(getEntityStatus()); | ||
| OSCEntityManager.update(em, this.jobRecord); | ||
| return null; |
| } catch (Exception e) { | ||
|
|
||
| // TODO remove when EM and TX are injected | ||
| log.error("Fail to update JobRecord " + this, e); |
There was a problem hiding this comment.
ahh...sorry missed the part about this being a TransactionalEntityManager
| @@ -57,37 +57,35 @@ public void onMessage(final String message) { | |||
| || eventType.contains(OsNotificationEventState.INTERFACE_DELETE.toString())) { | |||
| log.info(" [Port] : message received - " + message); | |||
| if (this.entity instanceof SecurityGroup) { | |||
| new TransactionalRunner<Void, Void>(new TransactionalRunner.ExclusiveSessionHandler()) | |||
| .withErrorHandling(getErrorHandler()).exec(getTranscationalAction(eventType, message)); | |||
|
|
|||
There was a problem hiding this comment.
can we refactor transactional runner/action to use the new txcontrol? or just eliminate it and use the new pattern in more generic way by reusing ErrorHandler for example
there are around 9 usages of TransactionalRunner and everywhere we will have the similar code
There was a problem hiding this comment.
TransactionalRunner was deleted in the first commit of this PR, and its usages refactored.
|
|
||
| } catch (Exception e) { | ||
| if(e instanceof ScopedWorkException) { |
There was a problem hiding this comment.
i see in some places we catch ScopedWorkException and others we do instance of.
can you please use a consistent pattern of either one?
There was a problem hiding this comment.
ScopedWorkException is a wrapping exception, similar to InvocationTargetException. You will see that on lines 74-81 we do some logging and alerting with the exception, but when it's ScopedWorkException we want to unwrap it first. If we had a separate catch block for it then we would have to either repeat lines 74-81 or pull them into a utility method.
However in examples such as TaskNode.java line 152, both catch blocks are simply one-liners, and it would add code to use the instanceof-unwrapping pattern.
There was a problem hiding this comment.
@neilxb i got that. My comment was to use a consistent approach of maybe doing the instance of everywhere for example in Job.java line 139, we catch scoped exception, unwrap it and do the same for catch exception.
} catch (ScopedWorkException e) {
// Unwrap the ScopedWorkException to get the cause from
// the scoped work (i.e. the executeTransaction() call.
log.error("Fail to update JobRecord " + this, e.getCause());
} catch (Exception e) {
// TODO:nbartlex remove when EM and TX are injected in A7
log.error("Fail to update JobRecord " + this, e);
There was a problem hiding this comment.
On consideration, I think it's better to explicitly catch the ScopedWorkException, and where the exception handling need to be duplicated refactor that handling code into a method.
If in the future a piece of code changes and can no longer throw ScopedWorkException, the IDE and compiler will give a warning for the catch block, but will not give a warning for the cast.
| } | ||
| } | ||
|
|
||
| log.info("Service response: " + response); | ||
| return response; | ||
| } | ||
|
|
||
| protected void chain(Callable<O> toCall) { |
There was a problem hiding this comment.
Will add javadoc as part of change discussed above.
| private EntityManager em = null; | ||
|
|
||
| private Callable<O> secondaryDispatch; |
There was a problem hiding this comment.
not really sure this is as flexible as the previous approach. we could call commitChanges multiple times and have as many dispatches as needed. can we do the same here? Also the secondary dispatch may not return any response. If it returns null, then the response is effectively overwrriten by the secondary dispatch
There was a problem hiding this comment.
Yes I discussed this with Tim – it would be quite simple to have an arbitrary number of chained dispatches. He didn't do this because there were no examples in the code that needed it. However since you would like the flexibility to do it in the future, I can make that change.
Regarding the return type, I think the best thing to do is change the type of the chained invocation from Callable<O> to Function<O,O>. That way, a chained invocation can see the previous response value and choose to either pass that through or substitute its own response.
| @@ -58,7 +58,7 @@ public O dispatch(I request) throws Exception { | |||
| throw new VmidcException(Server.PRODUCT_NAME + " server is in maintenance mode."); | |||
| } | |||
|
|
|||
| if (this.em == null || !this.em.isOpen()) { | |||
| if (this.em == null) { | |||
There was a problem hiding this comment.
I have confirmed that issue #100 does not occur in this branch.
No description provided.