Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deferred exceptions second attempt #598

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
4 participants
@sebplorenz
Copy link
Contributor

commented Feb 14, 2014

!XTS !QA_JTA !BLACKTIE !QA_JTS_JACORB !BLACKTIE

This is my second attempt for handling deferred exceptions. The first approach only worked for a single resource. This one works for multiple resources enlisted in a JTA transaction.

  1. I moved the ExceptionDeferrer interface to ArjunaJTA.
  2. XAOnePhaseResource and XAResourceRecord implement ExceptionDeferrer.
  3. Each time TransactionImple wraps an XAResource into an ExceptionDeferrer it adds the ExceptionDeferrer to a list.
  4. Before TransactionImple throws an exception during commit it will add all deferred exceptions from the ExceptionDeferrerS to the suppressed throwables of the exception to be thrown.
@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

Started testing this pull request with BLACKTIE profile on Linux: http://172.17.131.2/job/btny-pulls-narayana/PROFILE=BLACKTIE,jdk=jdk7.latest,label_exp=linux/402/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

Started testing this pull request with BLACKTIE profile on Windows: http://172.17.131.2/job/btny-pulls-narayana-windows2008/581/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

BLACKTIE profile tests failed on Windows - Narayana Failed http://172.17.131.2/job/btny-pulls-narayana-windows2008/581/

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

Started testing this pull request with BLACKTIE profile on Windows: http://172.17.131.2/job/btny-pulls-narayana-windows2008/582/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

BLACKTIE profile tests failed on Windows - Narayana Failed http://172.17.131.2/job/btny-pulls-narayana-windows2008/582/

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2014

The change looks good to me, will review CI once its complete. @mmusgrov, what do you think?

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

Started testing this pull request with BLACKTIE profile on Linux: http://172.17.131.2/job/btny-pulls-narayana/PROFILE=BLACKTIE,jdk=jdk7.latest,label_exp=linux/403/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Feb 14, 2014

public interface ExceptionDeferrer
{

List<Throwable> getDeferredThrowables();

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Feb 17, 2014

Contributor

A list is ordered. Does the order imply any guarantees - if yes then it would be nice to see that documented otherwise a Collection would suffice.

@@ -765,17 +783,25 @@ public boolean enlistResource(XAResource xaRes, Object[] params)
private AbstractRecord createRecord(XAResource xaRes, Object[] params, Xid xid)
{
final AbstractRecord record;

if (_exceptionDeferrers == null)

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Feb 17, 2014

Contributor

Every transaction is now maintaining an extra collection as part of its state. Would it be possible to create the collection at the point where an exception is about to be thrown by inspecting the failedList.in BasicAction (super class of this class) and then ask each member of the failed list to report any deferred exceptions.

I am also wondering whether the interface method List getDeferredThrowables(); could be rewritten as void getDeferredThrowables(List); so that the resource record does not have to always create a collection to maintain its own list of throwables.

This comment has been minimized.

Copy link
@sebplorenz

sebplorenz Feb 25, 2014

Author Contributor

I also don't like the idea that the transaction needs an extra collection. But I realized that the failedList in BasicAction does not always contain all failed records. E.g. if there is a ONE_PHASE_ERROR inside onePhaseCommit then the record is not added to failedList. In this case after the onePhaseCommit neither BasicAction nor TransactionImple has any reference to the failed record. That is why I introduced the list of ExceptionDeferrers in TransactionImple. I will investigate further to find a solution.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Feb 28, 2014

Contributor

You could maybe look into adding a method to BasicAction for reporting deferred exceptions. In the event that the edge case you mention occurs you would store the (single) exception in BasicAction state. When asked for the list of deferred exceptions you would see if you have the one phase error and return that, otherwise you would consult the failedList.

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2014

we just raised #606, if we merge that before this we should update this to addDeferredThrowable to the xa_end in topLevelOnePhaseCommit please

@mmusgrov

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2014

I think the solution looks good to merge (although I did wonder why two of the tests were commented out). This enhancement will be very useful for app developers and narayana maintainers alike, thanks for the contribution.

Caveat: I would like to see testCheckDeferredHeuristicRollbackSecondResourceFails passing before the code is committed to master.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

Started testing this pull request with BLACKTIE profile on Linux: http://172.17.131.2/job/btny-pulls-narayana/PROFILE=BLACKTIE,jdk=jdk7.latest,label_exp=linux/438/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

Started testing this pull request with BLACKTIE profile on Windows: http://172.17.131.2/job/btny-pulls-narayana-windows2008/592/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

Started testing this pull request with BLACKTIE profile on Linux: http://172.17.131.2/job/btny-pulls-narayana/PROFILE=BLACKTIE,jdk=jdk7.latest,label_exp=linux/439/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

BLACKTIE profile tests passed on Windows - Job complete http://172.17.131.2/job/btny-pulls-narayana-windows2008/592/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

Started testing this pull request with BLACKTIE profile on Windows: http://172.17.131.2/job/btny-pulls-narayana-windows2008/593/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

BLACKTIE profile tests passed on Windows - Job complete http://172.17.131.2/job/btny-pulls-narayana-windows2008/593/

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@jbosstm-bot

This comment has been minimized.

Copy link

commented Mar 7, 2014

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2014

Hi Seb,

Thanks so much for contributing this to the project. I am now going to merge it.

We look forward to any further input you can provide to our project in the future!
Tom

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2014

merged - thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.