TransactionTimeoutConfiguration pluggability

[jhalliday]

Adrian and the JBossAS team:

I'm fixing http://jira.jboss.com/jira/browse/JBAS-5081 for use in JBossAS 4.2 and will then port across to 5.0

I'm trying to get my head around how to write JBossAS testsuite tests, specifically one that will cover this.

I don't see any way in the AS to get the object that implements TransactionTimeoutConfiguration in a pluggable fashion. The best I can do is assume that the TransactionManager also implements it and cast appropriately. Do we need a TransactionTimeoutConfigurationLocator to live alongside of the TransactionManagerLocator?

Also, what do I do in the case of a new test like this that I know is going to fail until the next JBossTS release is available for consumption by the AS? Check it in anyhow and accept that the testsuite won't pass for a few weeks?

Thanks

[adrian.brock]

"jhalliday" wrote:
Adrian and the JBossAS team:

I'm fixing http://jira.jboss.com/jira/browse/JBAS-5081 for use in JBossAS 4.2 and will then port across to 5.0

I'm trying to get my head around how to write JBossAS testsuite tests, specifically one that will cover this.

I don't see any way in the AS to get the object that implements TransactionTimeoutConfiguration in a pluggable fashion. The best I can do is assume that the TransactionManager also implements it and cast appropriately. Do we need a TransactionTimeoutConfigurationLocator to live alongside of the TransactionManagerLocator?

It is an optional extension interface to the TransactionManager.
It also gets implemented by the JBossJCA TxConnectionManager
by forwarding it to the tm (if it implements it).

Also, what do I do in the case of a new test like this that I know is going to fail until the next JBossTS release is available for consumption by the AS? Check it in anyhow and accept that the testsuite won't pass for a few weeks?

Thanks

Either leave it failing with a "FIXME JBAS-5081" in the error message that the test produces.
Or if it is stopping a release add it to the "bad.tests.exclude" section.

[jhalliday]

> It is an optional extension interface to the TransactionManager.

In that case should it not be declared as

public interface TransactionTimeoutConfiguration extends TransactionManager

Right now nothing in the integration spi that forces implementations to use the the same object to provide both. Therefore making that assumption in the test is a bad thing.

> Either leave it failing with a "FIXME JBAS-5081" in the error message that the test produces.
> Or if it is stopping a release add it to the "bad.tests.exclude" section.

Well it is going into AS branch 4.2 initially and since we don't have any releases planned from that, leaving it failing should not be a problem. Trunk is more of a problem, I don't know if our new JBossTS release will be out before AS 5 CR1. Probably best to exclude it for that.

Thanks

[adrian.brock]

"jhalliday" wrote:
> It is an optional extension interface to the TransactionManager.

In that case should it not be declared as

public interface TransactionTimeoutConfiguration extends TransactionManager

No. Because the ConnectionManager only implements TTC not the full TM.

Right now nothing in the integration spi that forces implementations to use the the same object to provide both. Therefore making that assumption in the test is a bad thing.

If it doesn't implement the interface then don't run the test.

public void testSomething()
{
 Object tm = TransactionManagerLocate.locate();
 if (tm instanceof TransactionTimeoutConfiguration == false)
 return; // The interface is optional
}

But if you do implement the interface then it should "do what it says on the tin" ;-)

[jhalliday]

The tin actually contains no specification to cover this - it only requires that the exception is thrown if the tx is marked for rollback, not if it's actually rolled back or rolling back. Basically the interface spec does not anticipate async rollback processing. I could close the JIRA as 'this is not a bug' until the spi javadoc is revised. Or in light of your comment here, I could throw away the JBossTS implementation of TransactionTimeoutConfiguration and nothing in the AS would break?

[adrian.brock]

"jhalliday" wrote:
The tin actually contains no specification to cover this - it only requires that the exception is thrown if the tx is marked for rollback, not if it's actually rolled back or rolling back. Basically the interface spec does not anticipate async rollback processing. I could close the JIRA as 'this is not a bug' until the spi javadoc is revised.

The javadoc was written against the old TM behaviour it obviosuly needs updating.
RolledBack/Committed/Completed/etc. is also an error
(only RollbackOnly was possible with the old TM).

Or in light of your comment here, I could throw away the JBossTS implementation of TransactionTimeoutConfiguration and nothing in the AS would break?

Correct, except for people that expect

<set-tx-timeout/>

in the jdbc resource adapter to work like it used to.

Instead no query timeout will be set automatically (not fatal, but it is a regression
of sorts).

P.S. I've got a "feature request" open for JavaEE6 that this method or something
like it gets added to the TransactionSynchronizationRegistry (JTA spec).

* It will mean this behaviour is portable across appservers
* You'd have to implement it anyway ;-)

[jhalliday]

> Committed/Completed/etc. is also an error

That's more than a javadoc change. The only exception I can throw with the current method signature is RollbackException, which is hardly appropriate for those cases. The spi would have to change too. Fix for trunk maybe, I'll have to rev the spi anyhow for the new recovery handling.

> ... the jdbc resource adapter to work like it used to.

Right, so it's not actually optional if AS 4.2 is going to work as the users expect. Hence the test suite should not consider it optional. Throwing it away should cause a regression, since that breaks existing functionality.

[jhalliday]

I'm tempted to revise the spi spec such that the method throws SystemException if the arg is true and the tx is in anything other than STATUS_ACTIVE. The JIRA mentions STATUS_PREPARING as a valid state, but I'm not so sure. Attempting to timeout and rollback a PREPARING tx is a very dodgy proposition. IllegalStateException is probably more appropriate than SystemException, but unchecked. There is also the matter of clarifying the expected time units for the return value.

[adrian.brock]

"jhalliday" wrote:
I'm tempted to revise the spi spec such that the method throws SystemException if the arg is true and the tx is in anything other than STATUS_ACTIVE. The JIRA mentions STATUS_PREPARING as a valid state, but I'm not so sure.

Its perfectly valid for "one resource adapter" or synchronization to invoke another
resource adapter during the before synchronization or prepare
e.g. the hibernate rar (if it was used) delegates work to the jdbc rar.

Attempting to timeout and rollback a PREPARING tx is a very dodgy proposition.

Its no where near as bad during the commit phase. ;-)

IllegalStateException is probably more appropriate than SystemException, but unchecked. There is also the matter of clarifying the expected time units for the return value.

SystemException is for serious failure isn't it?

IllegalStateException would be preferable if it is not RollBackOnly or RolledBack.

The other bad states (e.g. Commiting/ted) indicate a misuse by the application.
e.g. forking threads and re-enlisting the tx in the new threads but not synchronizing
the commit across that work.

[jhalliday]

> SystemException is for serious failure isn't it?

Like being unable to determine the current timeout? :-)

int getTransactionTimeout() throws SystemException;

I picked SystemException to keep the API consistent, but I guess we could ignore that, or change getTransactionTimeout to throw IllegalStateException too.

Speaking of consistency, why does getTransactionTimeout() return int whilst getTimeLeftBeforeTransactionTimeout returns long?

[adrian.brock]

My turn to quote javadoc :-)

"The SystemException is thrown by the transaction manager to indicate that it has encountered an unexpected error condition that prevents future transaction services from proceeding."

Not really appropriate for a transient failure wrapped in an SQLExecption because
the tx has timed out or there's been a setRollbackOnly() due to an application
throwing a RuntimeException.

[adrian.brock]

SystemException is more for messages like

"I'm sorry, you're transaction log is corrupt, have a nice day. ;-)"

[jhalliday]

Quite. Rather begs the question, why does getTransactionTimeout throw it for any error? How about the case where there is no default timeout set? Is that an error and if not, what's the expected return value.

[jhalliday]

> I'm sorry, you're transaction log is corrupt, have a nice day. ;-)"

I'm going to get that printed on a t-shirt :-)

[adrian.brock]

"jhalliday" wrote:
Quite. Rather begs the question, why does getTransactionTimeout throw it for any error? How about the case where there is no default timeout set? Is that an error and if not, what's the expected return value.

-1 == not specified/unknown

But that's really for the case where you specify that the error should be ignored.

That usage only existed in the old TM to support the OTS Current.get_timeout()
and even there I didn't think it looked correct, but I'm not a big OTS expert. :-)