Clarify usage of LRAClient#getCurrent()

[xstefank]

In scenario when getCurrent is called repeatedly in the same method which starts new LRAs it's not clear whether we want behaviour where each newly started LRA represents the current context:

@LRA(LRA.Type.REQUIRES_NEW)
public Response get(@HeaderParam(LRAClient.LRA_HTTP_HEADER) String lraIdURL) throws MalformedURLException {
        URL lraId = new URL(lraIdURL);
        
        Assert.assertEquals(lraId, lraClient.getCurrent());
        
        URL newLRAId = lraClient.startLRA(null, "newTopLevelLRA", 0L, ChronoUnit.SECONDS);
        Assert.assertEquals(newLRAId, lraClient.getCurrent());

        URL secondLRA = lraClient.startLRA(null, "secondTopLevelLRA", 0L, ChronoUnit.SECONDS);
        Assert.assertEquals(secondLRA, lraClient.getCurrent());
        
        ...

or the method should always return the context which was started when the method was entered:

@LRA(LRA.Type.REQUIRES_NEW)
public Response get(@HeaderParam(LRAClient.LRA_HTTP_HEADER) String lraIdURL) throws MalformedURLException {
        URL lraId = new URL(lraIdURL);
        
        Assert.assertEquals(lraId, lraClient.getCurrent());
        
        URL newLRAId = lraClient.startLRA(null, "newTopLevelLRA", 0L, ChronoUnit.SECONDS);
        Assert.assertNotEquals(newLRAId, lraClient.getCurrent());
        Assert.assertEquals(lraId, lraClient.getCurrent());

        URL secondLRA = lraClient.startLRA(null, "secondTopLevelLRA", 0L, ChronoUnit.SECONDS);
        Assert.assertNotEquals(secondLRA, lraClient.getCurrent());
        Assert.assertEquals(lraId, lraClient.getCurrent());
        
        ...
    }

The javadoc states:

     * checks whether there is an LRA associated with the calling thread
     * (this method provides an alternative to relying on the presence
     * of the {@link LRAClient#LRA_HTTP_HEADER} HTTP header fields since
     * not all users of the API are being called in the context of JAX-RS
     * resource requests).

[mmusgrov]

The intent of getCurrent was that the returned LRA is the one that gets propagated on JAX-RS business requests and the last LRA that the client created (whether is was via an annotation or explicitly via the client API) that is still running is the one to be propagated. Doing anything else will only serve to create confusion in the mind of the developer.

Also if you force the business author to explicitly set the context on the JAX-RS header on business invocations then I would argue that we are no longer CDI first.

[xstefank]

@mmusgrov ok, but we need to rewrite javadoc for the method as it seems that getCurrent() should be replacing LRAClient#LRA_HTTP_HEADER. WDYT?

[mmusgrov]

Agreed, I will update the javadoc.

Also I would like to point out that I used to have a corresponding method called setCurrent to enable business methods to change the notion of the current active LRA. Do you think it is still okay not to have that method in the API?

[xstefank]

Can you come up with some use-case please?

[mmusgrov]

The business method invokes two different services (asynchronously) and wants the two invocations to run with a different context so he needs to creates 2 LRA's and then ensure that each invocation uses the correct context.

[xstefank]

In that case thread context won't get propagated in the async threads so it would be better to start LRAs in the individual async invocations directly. Maybe I don't understand what you mean.

[ochaloup]

Just from other perspective. Isn't a bit confusing the current behaviour in situation like this?

The LRA is started with LRAClient call (id2) and completed before the end of the method. Then the getCurrent returns non-existent id (from perspective of LRA lifecycle management).

@LRA
@Path("/order")
public void makeOrder() {
  em.persist(new Order());
  URL id1 = lraClient.getCurrent();
  URL id2 = lraClient.startLRA(...);

  // some longer lasting business logic here ...
  // meanwhile the LRA with id2 is completed, this code has no many chances
  // to find out that id2 was completed

  URL nonExistentId2 = lraClient.getCurrent();
 }

[mmusgrov]

    @LRA
    void bisinessMethod() {
        callAServiceInCurrentLRA();
        callAnotherServiceInNewLRA();
        callAnotherServiceInNewLRA();
    }

    void callAnotherServiceInNewLRA() {
        URL lra = lraClient.startLRA(null, "xxx", LRA_TIMEOUT_MILLIS, ChronoUnit.MILLIS);

        lraClient.setCurrent(lra);
        invokeService();
        lraClient.closeLRA(lra);
    }

[ochaloup]

I have another doubt, just how this should work. If LraClient is used for closing then the getCurrent reacts on that? I mean is the code below correct, aka. how it's supposed to work?

@LRA 
public void businessMethod() {
  URL id1 = lraClient.getCurrent();  // returns id1
  URL id2 = lraClient.startLRA(...); 
  lraClient.getCurrent();  // returns id2
  lraClient.closeLRA(...); 
  lraClient.getCurrent();  // returns id1
}

[mmusgrov]

Yes it is correct. The model is that each time a new LRA is started it is pushed onto a stack and each time an LRA is finished it is removed from the stack (not necessarily popped unless it was the last one to be pushed). The getCurrent() method will always return the last LRA that was pushed onto the stack. I think this model is the one that makes intuitive sense.

Notice that with this model there is nothing special about LRAs started and ended via annotations.

Also, given this model I thought it useful to also provide a setCurrent method but I was asked to remove it in one of our hangouts.

@ochaloup Does this explanation provide a satisfactory answer to both of your questions?

Finally, providing an operation to look at the stack is a possibility but I think that would be overkill.

[ochaloup]

For the first one, I understand it it's just up to developer to find out the state. For the second one, yes.

I think this model should not provide the setCurrent method. With getCurrent the LRA implementation can follow the started and finished LRAs (started either with annotations or via programmatic api). But adding the setCurrent sounds me like adding completely different capability of storage. Pushing whatever "unmanaged" LRA to the current seems to me just strange.

[ochaloup]

This point is not connected with the previous discussion.

If not said elsewhere else the specification should define that finished method annotated with the @LRA finishes the LRA started "by the annotation"

@LRA(end = true)
public void businessMethod() {
  URL id1 = lraClient.getCurrent(); // returns id1
  URL id2 = lraClient.startLRA(...);
} // finishing LRA id1, LRA id2 won't be finished

[mmusgrov]

@ochaloup Agreed the javadoc needs to make this clear. I will go ahead and raise a PR against this issue (using text similar to my last response) and then we can further discuss it there if required.

On the issue of setCurrent: the idea was that if the business method had started multiple new LRA's then setCurrent would enable him to choose which context he wants to be propagated on his next JAX-RS invocation which is a useful capability that is not possible without setCurrent. Putting an "unmanaged LRA" would be an error and would simply result in an exception/log warning. But if the consensus is that it is not required then I would reluctantly concede but the spec is diminished without it.

Furthermore, it is a specification goal to enable the developer to be able to structure his application using the features that LRA's provides:

Different usage patterns for LRAs are possible, for example LRAs may be used sequentially and/or
concurrently, where the termination of one LRA signals the start of some other unit of work within
an application. Additionally, speculative execution of work can be supported by nesting LRAs, some
of which can be cancelled independently of the parent LRA whilst others are closed based on the
outcome of other LRAs. LRAs are units of compensatable work and an application may have as
many such units of work operating simultaneously as it needs to accomplish its tasks. Furthermore,
the outcome of work within LRAs may determine how other LRAs are terminated. An application
can be structured so that LRAs are used to assemble units of compensatable work and then held in
the active state while the application performs other work in the scope of different (concurrent or
sequential) LRAs. Only when the right subset of work (LRAs) is arrived at by the application will
that subset be confirmed; all other LRAs will be told to cancel (complete in a failure state).

and being able to control which context gets propagated on different business requests facilitates the above goal and without the feature the business code that is structured in this way is likely to be more complex.

[mmusgrov]

@xstefank @ochaloup I updated my last comment with a reference to the spec to justify why setCurrent is useful. @tomjenkinson you were the one who asked for its removal, in light of my expanded reasoning for its inclusion are you still of the opinion that the feature is not useful.

[xstefank]

Personally I would still prefer explicit LRA id propagation so the user can choose which id will get propagated but that is different discussion.

For setCurrent use-case -- doing context propagation like this:

URL id1 = lraClient.startLRA(null, "id1", 0L, ChronoUnit.SECONDS);
URL id2 = lraClient.startLRA(null, "id2", 0L, ChronoUnit.SECONDS);
URL id3 = lraClient.startLRA(null, "id3", 0L, ChronoUnit.SECONDS);

...
        
lraClient.setCurrent(id1);
makeBusinessRESTCall();

...
        
lraClient.setCurrent(id3);
makeAnotherBusinessCall();
   
...
        
lraClient.setCurrent(id2);
makeAnotherBusinessCall2();
        
...

instead of:

URL id1 = lraClient.startLRA(null, "id1", 0L, ChronoUnit.SECONDS);
URL id2 = lraClient.startLRA(null, "id2", 0L, ChronoUnit.SECONDS);
URL id3 = lraClient.startLRA(null, "id3", 0L, ChronoUnit.SECONDS);

...
        
makeBusinessRESTCall(id1);

...
        
makeAnotherBusinessCall(id3);
   
...
        
makeAnotherBusinessCall2(id2);
        
...

where all business calls are just wrappers around basic JAX-RS Client API or preferably MP REST client seems to me a little redundant. User is still propagating the header but implicitly. So instead of doing

.header(LRAClient.LRA_HTTP_HEADER, lraId);

somewhere between the lines the user needs to do

lraClient.setCurrent(lraId);

AND then very similar call like the first one just omitting the header.

@mmusgrov I am sorry if I'm missing what you have in mind. However I still don't have a strong preference here. If we include it, it will be harder to remove it while on the other hand if we don't include it we can add it in 1.1 assuming community requests this functionality (we can throw it as an idea to the MP google group)

[ochaloup]

my two cents: my personal preference is that for the LRAClient#startLRA the explicit context propagation should be picked.

[mmusgrov]

Don't forget that this is a CDI first project.

[tomjenkinson]

If it is CDI first then couldn't both setCurrent and getCurrent be removed because you can't do that with annotations?

[mmusgrov]

An annotation based participant can get and set the active LRA via

@HeaderParam(LRA_HTTP_HEADER) String lraId

[rdebusscher]

Indeed, getCurrent and setCurrent can be removed.

[mmusgrov]

Ha ha, you are right, I have just argued that getCurrent and setCurrent are superfluous. In that case I agree that the methods can be removed.

[ochaloup]

to be honest I think some method similar to getCurrent is handy. Currently the LRAClient#getCurrent was the only way how to get the LRA id when LRA was stared by annotation.

When the LRAClient will be moved to 1.x release then this discussion could be purposeless just I wanted to express my objection here.

I beg for apologies I forget the same is currently possible to get from header.

[mmusgrov]

So @xstefank can we close this or do you want it left open to be discussed in the 1.x round of issues?

[xstefank]

@mmusgrov If we are to bring the LRAClient interface in the same form as it is right now to 1.x (once it is removed for 1.0) please do keep this issue open as there will be this question rising again once users will be able to use LRAClient in CDI beans where @HeaderParam annotation cannot be used.