Hi Michael,
thanks for reviewing these patches:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=320643
https://bugs.eclipse.org/bugs/show_bug.cgi?id=337291
I have submitted the associated changes. From my point of view, 320643 requires some further cleanup as the translation from EclipseLink OLEs to JPA OLEs should be handled in the JPA component/subproject and not in the RepeatableWriteUOW. Therefore, I left 320643 open.
Best regards,
Adrian
From: Michael Frank O'Brien [mailto:michael.obrien@xxxxxxxxxx]
Sent: Donnerstag, 17. Februar 2011 16:24
To : Goerler, Adrian
Subject: Re: bug 337291 and 320643 reviewed
Adrian,
Both bugs look good, I would check them in.
for
https://bugs.eclipse.org/bugs/show_bug.cgi?id=320643
1) catch/rethrow - I used to think this was not a good pattern - I was wrong in the case that - I quote "higher layers should catch lower-level exceptions and in thier place, throw exceptions that are explainable in terms of the higher level abstraction"
You are translating a lower level exception (EclipeLink) into a higher level (javax.persistence facing API)
However since we need to throw a javax exception in the batch case - this looks good
I consulted "Effective Java"- by Joshua Bloch to verify this. It looks like your pattern is good and is a variant of Item#43 p.178 of the book.
I also agree that we need to hide the JPA implementation in favor of the spec where possible.
https://bugs.eclipse.org/bugs/attachment.cgi?id=179243&action="">
2) The new assertExceptionWrapsOLE assertions in RollbackExceptions look good
3) I thought the tests for IllegalStateException on an attempted rollback in an OptimisticLockException were new - they are just shifted - but good
4) Tests for newly thrown javax OLE wrapped in the existing PersistenceException from an o.e.p.ole during a batch is good
I am tracking 320643 for any future refactor as I would be interested in this as well.
thank you
/michael
On 16/02/2011 11:57, Goerler, Adrian wrote:
Hi Michael,
thanks for looking into this. Not need to hurry. I am busy tomorrow afternoon until after the committers call. We could after that. If you plan to look into 320643 as well, just note that the tests from the last patch set are now moved to 337291.
-Adrian
Adrian,
I will look at it in 3 hours after meetings are done (1400EDT) and get back to you by 0900 EDT tomorrow (around 1500 your time).
I would like to go over the exception handling and specifically the OptimisticLockException handing.
thank you
/michael
On 16/02/2011 10:50, Goerler, Adrian wrote:
I am proposing to add some tests that run with JDBC-batching enabled. There is a related bug (320643) which I have opened a while ago, which addresses an issue with EclipseLink OLEs being thrown instead of JPA OLEs. I have not been able to find a satisfying solution for 320643 by now. But JDBC batching is important for us and I don’t want to miss the test coverage for this feature. Therefore, I am proposing to add the OLE-issue (320643) for now and ignore it in the JDBC-batching tests.
Could you please have a look?
