Skip to content
New issue

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

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

Already on GitHub? # to your account

[WFTC-75] unfinished XA commit records needs to be recorded for recovery #95

Conversation

ochaloup
Copy link
Contributor

@ochaloup ochaloup commented Aug 23, 2019

https://issues.jboss.org/browse/WFTC-75
https://issues.jboss.org/browse/JBEAP-17347

@fl4via would you be so kind and review this PR if it's right? What happens is that for failure on XA commit processing the registry does not return the XA resource during recovery. That makes the recovery not deleting the WFTC registry file.

When this will be approved I will create a PR for 1.1 branch. I would need this being fixed for CD18 (aka. JBEAP-17347)

/cc @chengfang

@@ -166,7 +166,7 @@ public void commit(final Xid xid, final boolean onePhase) throws XAException {
try {
if (commitToEnlistment()) lookup(xid).commit(onePhase);
} catch (XAException | RuntimeException exception) {
if (onePhase && resourceRegistry != null)
Copy link
Contributor

@fl4via fl4via Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ochaloup I remember why I added this onePhase check here. The line of thought is that, if the resource fails to commit and we are not preparing, just committing, we might need to track it.
Now, given that we need to track resource in doubt for two-phase as well, I wonder if the correct is the way you wrote it, or if it is just
if (resourceRegistry != null)
Wdyt would be correct here? Do you see a reason for not needing to add the resource to resource registry when the exception is thrown and one phase is true?

Copy link
Contributor Author

@ochaloup ochaloup Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fl4via ah, ok, I see. You are right. The both option needs the registry.

For 1PC the transaction manager does not hold any record on his own. But the remote resource (the remote server) could run the subordinate transaction. The WFTC needs to inform the transaction manager during call of recover() if there was some unfinished resources at the remote server. The record has to be saved for onePhase.

But what the call here really does is that it does **not save data persistently on disk but it just adds the resource to the resourceRegistry map. It marks the resources which are returned during recover() call when JVM crash does not happen.
If JVM crash happens then the file system record will be loaded during bootup and the resource will be returned during recover() call.

From the perspective of 2PC is more complicated. The 2PC is managed by transaction manager and when the failure happens the transaction manager holds data for recovery in the transaction log. Transaction manager should be capable to finish transaction even without data from the WFTC.
But the trouble is that the recover() call from WFTC does not return the resource (when there is no JVM crash) and thus the file system record is never removed.
In other words, when 2PC commit adds to the resource to the resourceRegistry then the recover() call can delete data from the file system. Even though the transaction recovery commit call does not need the WFTC record for finish correctly.

If you agree with my elaboration then we are in agreement that both 1PC and 2PC calls should add the resource to resourceRegistry on commit failure.

(I'm going to change the PR, please let me know what you think.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it makes sense to me :-)

@ochaloup ochaloup force-pushed the WFTC-75-delete-remote-records-on-commit-failure branch from d896a37 to 636ab61 Compare August 28, 2019 08:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants