ThreadLocals and invocation specific settings

[manik]

After our discussion on http://www.jboss.com/index.html?module=bb&op=viewtopic&p=3910399#3910399

it would seem that we need yet another ThreadLocal variable for the interceptors to query - this one containing references to the transaction and global transaction of the invocation.

This is in addition to 2 other ThreadLocal variables (currently in an inner class of TreeCache):

* ActionOrigin, a boolean
* Option - an config override for JBCACHE-106.

What do people think of the following, as a cleaner way to deal with all of the above:

* Create an InvocationContext object, which has references to a tx, a gtx, an actionOriginLocal boolean, and an Option object.
* The InvocationContext is accessed by static methods on the Interceptor class: getInvocationContext(), setInvocationContext().
* The InvocationContext is stored in ThreadLocal for now, could be later modified to pass this around as a part of the payload.

Thoughts, comments?

[manik]

Thinking some more about this, the InvocationContext can be the basis of a payload based invocation later on. So just rather than using Interceptor.getInvocationContext(), the InvocationContext will become a parameter of Interceptor.invoke() which makes for a clear migration path such that the rest of the interceptor code can now be written using an InvocationContext and this would not need to change later.

Thoughts?
Manik

[belaban]

Hmm, not so happy with this. IMO, InvocationContext should be an instance variable of Invocation (a.k.a. MethodCall), so

Object invoke(MethodCall c) throws Throwable {
 GlobalTransaction gtx=c.getValue("TRANSACTION");
}

or

Object invoke(MethodCall c) throws Throwable {
 GlobalTransaction gtx;
 gtx=c.getInvocationContext().
 getValue("TRANSACTION");
}

but not

Object invoke(MethodCall c, InvocationContext ctx)
 throws Throwable {
 GlobalTransaction gtx=ctx.getValue("TX");
}

If we don't need InvCtx, let's not introduce it.

[manik]

Fair enough, I agree that we shouldn't add parameters to the invoke() method unnecessarily, but I do like the idea of a typed object as a container for information specific to an invocation.

Perhaps even something like this:

invoke(InvocationContext ctx)
{
 MethodCall call = ctx.getMethodCall();
 Transaction tx = ctx.getTransaction();
 GlobalTransaction gtx = ctx.getGlobalTransaction();
 Option optionOverrides = ctx.getOptionOverrides();
 ...
}

[manik]

The downside is that it does change the sig of invoke() which may be a problem.

Turning it around, like you mentioned, we could do:

invoke(MethodCall call)
{
 InvocationContext ctx = call.getInvocationContext();
 Transaction tx = ctx.getTransaction();
 GlobalTransaction gtx = ctx.getGlobalTransaction();
 Option optionOverrides = ctx.getOptionOverrides();
 ...
}

But this isn't as elegant from a JBossCache point of view since the MethodCall becomes the focus of each invocation as opposed to an InvocationContext which contains the method call and orthogonal information around it.

But then this just boils down to a style thing. :-) Both mechanisms would work just as well and just as clean.

[manik]

Sorry, I just realised that in the second example, InvocationContext would have to be a part of MethodCall, and hence a part of JGroups and not JBossCache. So to propagate this stuff it would have to be a part of the MethodCall.

Just not happy with the MethodCall.getInvocationContext().getValue(String s) ... I prefer strongly typed objects but I understand why you need to keep it generic.

Lets see though, perhaps if we subclassed MethodCall - JBossCacheMethodCall - which added strongly typed methods such as JBossCacheMethodCall.getInvocationContext().getGlobalTransaction()?.

And this in turn would call getValue("GTX") so the weakly typed calls are in one place.

What do you think?

[belaban]

No, the MethodCall needs to be at the center of the universe, similar to org.jboss.mx.Invocation.
So your code above would be written as follows:

invoke(MethodCall call)
{
 Transaction tx = call.getTransaction();
 GlobalTransaction gtx = call.getGlobalTransaction();
 Option optionOverrides = call.getOptionOverrides();
 ...
}

This is 100% in line with org.jboss.mx.Invocation, and makes transitioning to Invocations much easier once we are ready to do it.
There is no need for an InvocationContext in this scenario.
BUT... since we don't have the payload in MethodCall yet, I propose the InvocationContext on the interceptor as described above. Once we switch to payloads (either in MethodCall or Invocation), IC needs to go as well.

[manik]

Yup, agreed. Makes sense.

InvocationContext will just be a temporary construct until MethodCall gains payload, and then the ability to achieve the same effect.

[belaban]

Yes. And there is a nice transitioning from MethodCall --> Invocation, which is the ultimate goal.
The MethodCall class is a dependency on JGroups, which I like :-), but which is not nice from an engineering standpoint.
So even if we run in LOCAL mode, jgroups.jar still needs to be referenced.
We'll get rid of this in the 1.4/1.5 release timeframe

[manik]

We're sorted then. :-)