[WFTC-38] Add a XAResourceRegistry to record in a file all in doubt r… #40
Conversation
| @@ -116,6 +116,31 @@ public int compareTo(final SimpleXid o) { | |||
| return res; | |||
| } | |||
|
|
|||
| public String toHexString() { | |||
There was a problem hiding this comment.
This method is used to name the file. Since the file name is not used for anything, we could alternatively use SimpleXid.toString() to name that file and remove this toHexString small refactoring
| @@ -82,6 +96,26 @@ | |||
| @Message(value = "Failure on running doRecover during initialization") | |||
| void doRecoverFailureOnIntialization(@Cause Throwable e); | |||
|
|
|||
| @LogMessage(level = Logger.Level.TRACE) | |||
There was a problem hiding this comment.
Added those methods as trace, let me know if you prefer DEBUG.
There was a problem hiding this comment.
TRACE is the correct choice here.
| @@ -728,13 +747,36 @@ public Builder setTransactionSynchronizationRegistry(final TransactionSynchroniz | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Do I also need to add getter methods for these two properties in Builder?
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| @Override |
There was a problem hiding this comment.
Please review: is SystemException the best option here? I could go with IOException as well, but that would force XAOutflowedResources to treat that exception or to declare an extra exception, I assumed that from the pov of the XAOutflowedResources caller it made more sense SystemException
There was a problem hiding this comment.
Yes, I think that SystemException is correct. We want Transaction.enlistResource to fail with an exception when an exceptional state occurs.
| * @throws XAException if there is a problem deleting the registry file | ||
| */ | ||
| @Override | ||
| protected void removeResource(XAResource resource) throws XAException { |
There was a problem hiding this comment.
Please verify correctness of XAException, hopefully I got it right
| assert fileChannel != null; | ||
| try { | ||
| assert fileChannel.isOpen(); | ||
| fileChannel.write(ByteBuffer.wrap(uri.toString().getBytes())); |
There was a problem hiding this comment.
Need to double-check that this is OK in a multithreaded setting.
There was a problem hiding this comment.
No delimiter is added so it will be difficult to separate URIs on read.
There was a problem hiding this comment.
We're relying on the default JDK character set encoding here; we should use StandardCharsets.UTF_8 instead.
| * | ||
| * @author Flavia Rainone | ||
| */ | ||
| final class JBossXAResourceRegistry extends XAResourceRegistry { |
There was a problem hiding this comment.
How about FileSystemXAResourceRegistry, and then it can be public?
| /** | ||
| * The xa recovery path, i.e., the path containing {@link #RECOVERY_DIR}. See {@link #setRelativeToPath}. | ||
| */ | ||
| private static Path xaRecoveryPath = FileSystems.getDefault().getPath(RECOVERY_DIR); |
There was a problem hiding this comment.
This state definitely should not be static.
There was a problem hiding this comment.
I think what we really want is one instance per provider rather than one instance per transaction. The individual transaction files could be tracked via a ConcurrentMap<SimpleXid, Entry> where Entry tracks the individual file state and information. The per-provider instance would contain the global config (base recovery path). The set of open file paths can be derived from the values of the map.
The XID would have to be given as an argument to the registry methods to make this work.
| recoverInDoubtRegistries(provider); | ||
| } catch (IOException e) { | ||
| // ignore with a logged message | ||
| Log.log.unexpectedExceptionOnXAResourceRecovery(e); |
There was a problem hiding this comment.
I think this should probably throw an exception, like an IllegalStateException perhaps which wraps the IOException
| * @return a newly-created resource representing a previously lost XA resource. | ||
| */ | ||
| protected XAResource reloadInDoubtResource(URI uri, String nodeName) { | ||
| return new SubordinateXAResource(uri, nodeName, this); |
There was a problem hiding this comment.
This should set the flags argument to FL_COMMITTED | FL_CONFIRMED.
| throw exception; | ||
| } | ||
| if (resourceRegistry != null) | ||
| resourceRegistry.removeResource(this); |
There was a problem hiding this comment.
Did we determine that it was safe to remove entries after prepare? Does the TM log the resource at this point?
There was a problem hiding this comment.
Yes, it does, according to the discussion we had last year with Tom. Anyway, I'll double check with him.
| throw Log.log.deleteXAResourceRecoveryFileFailed(XAException.XAER_RMERR, filePath, resource, e); | ||
| } | ||
| Log.log.xaResourceRecoveryFileDeleted(filePath); | ||
| } |
There was a problem hiding this comment.
Let's remember to discuss whether it's safe or optimal to leave entries for resources which are no longer in doubt.
… files all in doubt resources, and make recovery available using org.jboss.tm.XAResourceRecoveryRegistry
|
@dmlloyd the PR is updated |
…esources, and make recovery available using org.jboss.tm.XAResourceRecoveryRegistry