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

JBTM-2584 commit LRCO and a readonly resource #964

Merged
merged 1 commit into from Dec 18, 2015

Conversation

Projects
None yet
4 participants
@mmusgrov
Copy link
Contributor

commented Dec 16, 2015

https://issues.jboss.org/browse/JBTM-2584

!BLACKTIE !QA_JTA !QA_JTS_JACORB !QA_JTS_JDKORB !QA_IBMORB !PERF
!XTS

@@ -0,0 +1,193 @@
/*

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 16, 2015

Contributor

Maybe try to clean up this header in case we copy it again :)

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

Test looks good, reviewing change

@@ -1831,6 +1831,10 @@ protected synchronized final void phase2Commit (boolean reportHeuristics) throws

phase2Abort(reportHeuristics);
}
else if ((preparedList == null || preparedList.size() == 0) && actionStatus == ActionStatus.ABORTED) {
// the prepared list was completely processed but some resource rolled back
phase2Abort(reportHeuristics);

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 16, 2015

Contributor

Would be good to get a warning (like the pending list check prior)

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 16, 2015

Author Contributor

This is a scenario where one of the resources rolls back during the prepare phase but there are no heuristics which I think is just part of the normal behaviour of committing resources. A similar situation occurs when the first resource aborts during commit so we abort the remaining participants but we don't log a warning in this case. Here is where we abort the remaining participants

If you still think such a scenario is sufficiently noteworthy then I will add a WARN message.

On the other hand the pending list means that there are resources for which prepare was never called and this is unusual so is reported with a warning.

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

Apart from those, change looks good to me thanks

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

Left it ready for review until at least the logging message is added/disputed

@mmusgrov mmusgrov assigned tomjenkinson and unassigned mmusgrov Dec 16, 2015

@nmcl

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

OK, I get that but as the comment on line 1524 shows ...

                    phase2Commit(reportHeuristics); /* first phase succeeded */

phase2Commit should only be called if prepare has succeeded. In this example prepare clearly hasn't succeeded so we shouldn't be calling phase2Commit in the first place. Seems to me that the test above ...

if (prepare(reportHeuristics) == TwoPhaseOutcome.PREPARE_NOTOK)

needs changing instead so it also checks for this situation and we fall into the first block and call phase2Abort immediately.

BTW the test at the start of phase2Commit for the non-null pendingList is there more as a pre/post condition check in case a derived class calls this method when it really shouldn't, i.e., something breaks the protocol (is invalid). What you're describing is valid within the 2PC protocol but we simply have not checked for it in the first place.

@mmusgrov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

OK that make good sense. I will move the check to where you propose.

@nmcl

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

Maybe it's worth adding something to the javadoc/comments for phase2Commit to call out this requirement to, i.e., that you should only call it if prepare succeeded? It's kind of implied in the comment already but probably not explicit enough.

@mmusgrov mmusgrov force-pushed the mmusgrov:JBTM-2584 branch 2 times, most recently from 4f5a4c8 to 92b156b Dec 17, 2015

@mmusgrov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2015

I have moved the check to immediately after the prepare call.

@nmcl please can you take a look
@tomjenkinson putting the check here will output the existing warn_coordinator_BasicAction_36 message

int prepareStatus = prepare(reportHeuristics);

if (prepareStatus == TwoPhaseOutcome.PREPARE_NOTOK
|| (prepareStatus == TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 17, 2015

Author Contributor

I think I should be changing the prepare instead to return TwoPhaseOutcome.ONE_PHASE_ERROR instead of PREPARE_ONE_PHASE_COMMITTED if the onePhaseCommit aborts.

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 17, 2015

Contributor

One option is to change onePhasePrepare API to return the right status https://github.com/jbosstm/narayana/blob/master/ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/BasicAction.java#L2322. That would be a public API change so not advisible for this release but might be something to consider for future.

How about returning TwoPhaseOutcome.PREPARE_NOTOK if the 1PC fails (determined by actionStatus in the place you are currently proposing)? We lose resolution as to the cause of the failure but it minimizes the changeset and the new return code would not actually be used anyway except in your changeset.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 17, 2015

Author Contributor

You are looking at an outdated diff (sorry). I changed prepare to return
the correct status, as in:

return actionStatus == ActionStatus.ABORTED ? TwoPhaseOutcome.ONE_PHASE_ERROR : TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED;

On Thu, Dec 17, 2015 at 10:54 AM, Tom Jenkinson notifications@github.com
wrote:

In
ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/BasicAction.java
#964 (comment):

@@ -1498,7 +1498,11 @@ protected synchronized int End (boolean reportHeuristics)
}
else
{

  •            if (prepare(reportHeuristics) == TwoPhaseOutcome.PREPARE_NOTOK) {
    
  •            int prepareStatus = prepare(reportHeuristics);
    
  •            if (prepareStatus == TwoPhaseOutcome.PREPARE_NOTOK
    
  •                    || (prepareStatus == TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED
    

One option is to change onePhasePrepare API to return the right status
https://github.com/jbosstm/narayana/blob/master/ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/BasicAction.java#L2322.
That would be a public API change so not advisible for this release but
might be something to consider for future.

How about returning TwoPhaseOutcome.PREPARE_NOTOK if the 1PC fails
(determined by actionStatus in the place you are currently proposing)? We
lose resolution as to the cause of the failure but it minimizes the
changeset and the new return code would not actually be used anyway except
in your changeset.


Reply to this email directly or view it on GitHub
https://github.com/jbosstm/narayana/pull/964/files#r47892593.

Michael Musgrove
Transactions Team
e: mmusgrov@redhat.com
t: +44 191 243 0870

Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham (US), Paul Hickey (Ireland), Matt Parson
(US), Charles Peters (US)

Michael Cunningham (US), Charles Peters (US), Matt Parson (US), Michael
O'Neill(Ireland)

@mmusgrov mmusgrov force-pushed the mmusgrov:JBTM-2584 branch from 92b156b to 4f2043e Dec 17, 2015

@@ -2147,7 +2151,8 @@ protected synchronized final int prepare (boolean reportHeuristics)

ActionManager.manager().remove(get_uid());

return TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED;
return actionStatus == ActionStatus.ABORTED

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 17, 2015

Contributor

On the current diff now :)

One option is to change onePhaseCommit API instead to return the right status https://github.com/jbosstm/narayana/blob/master/ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/BasicAction.java#L2322. That would be a public API change so not advisible for this release but might be something to consider for future.

@@ -2147,7 +2151,8 @@ protected synchronized final int prepare (boolean reportHeuristics)

ActionManager.manager().remove(get_uid());

return TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED;
return actionStatus == ActionStatus.ABORTED
? TwoPhaseOutcome.ONE_PHASE_ERROR : TwoPhaseOutcome.PREPARE_ONE_PHASE_COMMITTED;

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 17, 2015

Contributor

How about returning TwoPhaseOutcome.PREPARE_NOTOK if the 1PC fails so keep the code on line 1501 as is?

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 17, 2015

Contributor

Actually, I prefer your approach its more accurate (but I would still wonder whether a onePhaseCommit API change might be cleaner in future)

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 17, 2015

Author Contributor

I disagree since the subclass can call BasicAction#status () to determine the result of calling onePhaseCommit.

Although that would not help if we are calling prepare/commit in asynchronous mode

This comment has been minimized.

Copy link
@tomjenkinson

tomjenkinson Dec 17, 2015

Contributor

Disagreeing with preferring your approach?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Dec 17, 2015

Author Contributor

lol I should have put it in parenthesis

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

@tomjenkinson

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2015

Looks good to me and addresses Marks comment

@mmusgrov mmusgrov merged commit 4f2043e into jbosstm:master Dec 18, 2015

@mmusgrov mmusgrov deleted the mmusgrov:JBTM-2584 branch Dec 18, 2015

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.