JBossDeveloper

Can the ThreadLocal stuff be replaced with using the payload in the new JGroups 2.2.9 MethodCall when it is final ?
Or is the common agreement that ThreadLocals are the way to go ?

I will vote for the payload with Invocation object embeded in there. We will need it for other purposes as well, e.g., maybe authentication/security in the future anyway?

The good thing is that once TxInterceptor fetches the TX from the TXManager, we just stuff it into the payload and don't have to fetch it again. But this creates a dependency on JGroups 2.2.9. In the future I would want to replace MethodCall with Invocation anyway

I think a payload and Invocation object, as Ben mentioned, is the way to go.

ThreadLocal is just a temporary solution until we can change the API, deprecate passing up MethodCalls in Interceptor.invoke(), etc. Temporary because we will probably not have payloads, Invocation objects till 2.0? I presume this is the timeframe in mind?

Is this common consensus then?

* For now, use thread local to hold tx and gtx for each invocation
- thread local object resides as a static in Interceptor
- set by the tx interceptor upon each invocation
- all other interceptors retrieve tx and gtx stuff from thread local

* Later on, move to an Invocation object that contains all specifics of an invocation.

#1 Why do we need a thread local for TX/GTX ? I say we don't change anything and simply call TxManager.getTransaction(), which uses a ThreadLocal itself. Once we have the invocation payload in MethodCall, we can switch to using the payload directly, rather than taking the detour over ThreadLocals.

#2 I understand you use ThreadLocals for Options. But I assume the Option parameter is exposed at the TreeCache API, e.g. in put() methods. Correct ?

#1 - this is fine in simplistic cases where we just need access to the TX. In the case of the GTX though, there are several ways to get a hold of it:

- TreeCache.getCurrentTransaction() - gets the existing gtx if there is one, creates a new one otherwise.
- txTable.get() - if the gtx has been registered
- getting the gtx from a method call args
- getting the gtx from a locally cached copy (as per Brian's fix for JBCACHE-359).

This makes interceptor code very unreadable as the gtx seems to come from several places and interceptors are not consistent in how and where they get a gtx reference.

I propose that the TxInterceptor goes through all the above ways of retrieving the gtx depending on the method call, and places this in a ThreadLocal accessible via the Interceptor so that all other interceptors don't need to care where they get the gtx from - it is already decided and set by the TxInterceptor.

Now this doesn't *have* to be in a separate ThreadLocal - we could use any of the existing mechanisms, I'd just like to stick to a standardised mechanism and have the TxInterceptor sort out any differences if for a particular call, the gtx needs to come from somewhere else.

Also see http://www.jboss.com/index.html?module=bb&op=viewtopic&t=73645

#2 - correct, these are exposed in the TreeCache api. Rather than adding more Method objects in the TreeCache for the interceptors to trap and extract args out of though, I put the options in a thread local and let the interceptors get it from there just to make the interceptor code clearer since, after all, this parameter controls an overriding behaviour, an aspect if you will, and not really data used in the operation.

E.g., my get(fqn, option) method looks like:

// put option in thread local
// call get(fqn)
// reset option in thread local

Again, see the discussion on InvocationContexts, which is a clean way of managing such invocation specific details.

http://www.jboss.com/index.html?module=bb&op=viewtopic&t=73645

"manik.surtani@jboss.com" wrote:
#1 - this is fine in simplistic cases where we just need access to the TX. In the case of the GTX though, there are several ways to get a hold of it:

- TreeCache.getCurrentTransaction() - gets the existing gtx if there is one, creates a new one otherwise.
- txTable.get() - if the gtx has been registered
- getting the gtx from a method call args
- getting the gtx from a locally cached copy (as per Brian's fix for JBCACHE-359).

This makes interceptor code very unreadable as the gtx seems to come from several places and interceptors are not consistent in how and where they get a gtx reference.

I propose that the TxInterceptor goes through all the above ways of retrieving the gtx depending on the method call, and places this in a ThreadLocal accessible via the Interceptor so that all other interceptors don't need to care where they get the gtx from - it is already decided and set by the TxInterceptor.

Now this doesn't *have* to be in a separate ThreadLocal - we could use any of the existing mechanisms, I'd just like to stick to a standardised mechanism and have the TxInterceptor sort out any differences if for a particular call, the gtx needs to come from somewhere else.

Also see http://www.jboss.com/index.html?module=bb&op=viewtopic&t=73645

"manik.surtani@jboss.com" wrote:
#2 - correct, these are exposed in the TreeCache api. Rather than adding more Method objects in the TreeCache for the interceptors to trap and extract args out of though, I put the options in a thread local and let the interceptors get it from there just to make the interceptor code clearer since, after all, this parameter controls an overriding behaviour, an aspect if you will, and not really data used in the operation.

E.g., my get(fqn, option) method looks like:

// put option in thread local
// call get(fqn)
// reset option in thread local

Again, see the discussion on InvocationContexts, which is a clean way of managing such invocation specific details.

http://www.jboss.com/index.html?module=bb&op=viewtopic&t=73645

Re #1; I don't think this is as complex or big a change, I can probably have it done by the end of today.

- Just a case of adding the ThreadLocal variables
- adding some code in the TxInterceptor to set these values
- search for all other direct uses of tx and gtx in the other interceptors and replace them with this call.

Also, I think it will help Brian get his JBCACHE-359 fix in.

Re #2; I agree that this will become a part of the payload. Migration should not be that hard since this is accessed by interceptors by calling a method on the abstract Interceptor. So that method will be all that we'd have to change.

And you're right, I do need to document this.

"manik.surtani@jboss.com" wrote:
Re #1; I don't think this is as complex or big a change, I can probably have it done by the end of today.

- Just a case of adding the ThreadLocal variables
- adding some code in the TxInterceptor to set these values
- search for all other direct uses of tx and gtx in the other interceptors and replace them with this call.

Also, I think it will help Brian get his JBCACHE-359 fix in.

public Object invoke(MethodCall call) throws Throwable {
 GlobalTransaction gtx=getInvocationContext().get("GTX");
 int replMode=getInvocationContext().get("repl_mode");
 ...
}

public Object invoke(MethodCall call) throws Throwable {
 GlobalTransaction gtx=call.get("GTX");
 int replMode=call.get("repl_mode");
 ...
}

Yeah, that would do it ... we're getting close, now we just need to think of a way of making it more strongly typed! :-)

Like I suggested in http://www.jboss.com/index.html?module=bb&op=viewtopic&p=3911423, perhaps a JBossCacheMethodCall subclass that would add strongly typed helper methods to extract specific objects out of the payload.