issue35 Do not define JAX-RS annotations on @Compensate methods

[xstefank]

resolves #35

Signed-off-by: xstefank [email protected]

[mmusgrov]

Nice work. I did have a couple of minor points but the contribution is otherwise approved.

[mmusgrov]

I only checked what you added or changed. Did you check the javadocs to make sure they are still accurate, I just took a look at @Compensate for example

[xstefank]

I will reiterate javadoc once more if I can, I was doing this for some quite time

[rdebusscher]

A few questions. We can handle them also on separate issues if we agree that a change is required.

[rdebusscher]

This is then something for 1.x already, as we haven't defined yet how you can supply these values? #15

[xstefank]

thank you, I must have missed that we've moved it to 1.x. I will replace String userData with URI parentLraId and will require right order of arguments

[mmusgrov]

Still +1 on the latest changes you pushed.

The javadoc for @compensate and @complete only requires a small modification. Something like:

 * If the annotated method is a JAX-RS resource method the id of the currently
 * running LRA can be obtained by inspecting the incoming JAX-RS headers and if
 * this LRA is nested then the parent LRA MUST be present in the header with the name
 * {@link org.eclipse.microprofile.lra.annotation.ws.rs.LRA#LRA_HTTP_PARENT_CONTEXT_HEADER}.
 *
 * If the annotated method is not a JAX-RS resource method the id of the currently
 * running LRA can be obtained by adhering to a predefined method signature as
 * defined in the LRA specification document. Similarly the method may determine
 * whether or not it runs with a nested LRA by providing a parameter to hold the parent id.
 * For example,
 * <pre>
 * @Compensate
 * public String compensate(URI lraId, URI parentId) { ...}
 * </pre>
 * would be a valid compensation method declaration.

[xstefank]

@mmusgrov thanks, I am in a middle of javadoc adjustments. I will use your suggestion.

[mmusgrov]

@xstefank Rudy raised a interesting point about reactive support for supporting async so I think we need the signature to optionally support returning CompletionStage<type>

[mmusgrov]

@mmusgrov you are right, the implementation is responsible for calling compensate method when it's not bound to JAX-RS so we can definitely implement CompletionStage as supported return type. Would that be all? I know there are several other "reactive" types that can be returned in rxJava, reactive streams etc. The question is what everything we want to cover in the spec.

Let's align as closely as possible with JAX-RS and use CompletionStage<type>

[mmusgrov]

@xstefank Will you squash your commits please

[xstefank]

@mmusgrov done, I was waiting for the final reviews but it seems this is good to go

[mmusgrov]

@xstefank I have temporarily removed my approval. My understanding was that the proxy would implement the @forget and @Status methods (like we currently have in the LRAManagement API) but your PR indicates otherwise.

So I removed the approval while I think about the various recovery scenarios to find a reason why we would still need @forget when this method is called locally. Similarly, I am not certain we need @Status. I assume that you have added them because you have thought of situations where they cannot be provided by the (proxy) implementation.

[xstefank]

@mmusgrov the proxy doesn't have semantic knowledge about what is going on inside of the participant. If @Compensate just returns ParticipantStatus.Compensating or Response with HTTP status 202 the proxy forwards this information to LRA coordination but when @Status is called it doesn't have any option of how to derive the status of the participant other than asking it by invoking its @Status method (or @Compensate again if there is no status provided by participant).

LRAManagement could derive status because what was returned from complete/compensate was Future which status could be checked for completion by proxy.

Side note, LRAManagement is still to be removed for 1.0 right? I will raise an issue if there is none once you confirm this.

[mmusgrov]

If the @Compensate method wants to return ParticipantStatus.Compensating it should return CompletionStage<ParticipantStatus>.

If it subsequently crashes then I am thinking that the implementation should be allowed to call the compensate method again (since idempotency is really an issue for distributed systems not in-VM calls). But I want to have a think about these failure scenarios to see if it is possible to remove the need for an @Status method.

[xstefank]

@mmusgrov I thought that the use of CompletionStage is optional, same as it is in JAX-RS. But even if this was required isn't CompletionStage return type in JAX-RS going to wait for the result so the Response filters can be applied? Maybe I am missing something.

Can we discuss this please on gitter as it's getting long and the PR is not the right place to hold this discussion.

[mmusgrov]

@nicolaferraro This PR has significant overlap with the current LRAManagment/LRAParticipant API which we would therefore like to remove. Instead we now support compensation via the standard Java annotations: @Compensate and @Forget and, optionally, @Complete and @Status. But note that the method annotated with @LRA still needs to be a JAX-RS endpoint.

We will call @Compensate as usual when the LRA is cancelled. The method must conform to one of the predefined signatures mentioned in this PR which must be validated by an annotation processor when the application is deployed. If you cannot compensate immediately then you can either return an instance of CompletionStage that we will use to monitor your progress, otherwise we will monitor your progress by calling the @Status method. After a crash we will call the status method if it's present otherwise the implementation will re-invoke the compensate method. If you report FailedToCompensate you need to remember that you failed until we call the @Forget method.

Please can you verify that it is okay to remove LRAManagment/LRAParticipant?

[mmusgrov]

@xstefank Can you add a test for participants that return Completion Stages please. An example of a compensation method that returns such a result could be:

    @Compensate
    public CompletionStage<ParticipantStatus> compensate(URI lraId) {
        CompletableFuture<Void> memUpdate = CompletableFuture.runAsync((() -> {/**/}));
        CompletableFuture<Void> dbUpdate = CompletableFuture.runAsync((() -> {/**/}));

        CompletableFuture<ParticipantStatus> stage1 = memUpdate.handle((s, e) -> e != null ? FailedToCompensate : Compensated);
        CompletableFuture<ParticipantStatus> stage2 = dbUpdate.handle((s, e) -> e != null ? FailedToCompensate : Compensated);

        return stage1.thenCombine(stage2, (s1, s2) -> {
            if (s1 == Compensated && s2 == Compensated)
                return Compensated;

            return FailedToCompensate;
        });
    }

[xstefank]

@mmusgrov TCK with CS added

[mmusgrov]

I know that I am giving more comments than on my first review but there have been javadoc updates since then and while reviewing them I noticed some (not huge) issues with the spec updates.

First some general observations:

The annotation for @compensate says that 412 Precondition Failed can be returned. What is the equivalent behaviour for non JAX-RS compensation.

The javadoc for @compensate refers the reader to the spec if the signature is invalid. To keep the javadoc self contained it would help to include a javadoc @link to InvalidLRAParticipantDefinitionException which states which signatures are valid. Similarly for @complete, @forget and @Status.

What is your opinion about issue #146 (Allow JAX-RS endpoints Complete/Compensate to be invoked using any HTTP method). If we allow that we don't need any javadoc text for it (for example you removed the @delete requirement from the javadoc for @forget and @get from @Status which implies that you also found it a problem having to keep javadoc and spec doc in sync).

The @Status method returns null if the participant does not know about the LRA. Since a null value or a status are valid why aren't you using java.util.Optional (I believe it was added to the language for this kind of purpose).

Sections 3.2.10 (@Status) and 3.2.11 (@forget) imply that the annotated method should be a JAX-RS endpoint (which is no longer the case).

The remainder of my comments are inline.

NB I also added a general comment on the issue description.

[xstefank]

@mmusgrov thank you for the review, comments inline:

The annotation for @compensate says that 412 Precondition Failed can be returned. What is the equivalent behaviour for non JAX-RS compensation.

The javadoc for LRA annotations states The annotation <b>SHOULD</b> be applied to JAX-RS annotated methods otherwise it <b>MAY</b> have no effect.. Therefore if implementation allows LRA on non-JAX-RS methods it should be implementation detail how such situations are handled? In every case, we can require to throw InvalidLRAParticipantDefinitionException if such case is detected if you prefer.

The javadoc for @compensate refers the reader to the spec if the signature is invalid. To keep the javadoc self contained it would help to include a javadoc @link to InvalidLRAParticipantDefinitionException which states which signatures are valid. Similarly for @complete, @forget and @Status.

+1

What is your opinion about issue #146 (Allow JAX-RS endpoints Complete/Compensate to be invoked using any HTTP method). If we allow that we don't need any javadoc text for it (for example you removed the @delete requirement from the javadoc for @forget and @get from @Status which implies that you also found it a problem having to keep javadoc and spec doc in sync).

I think #146 is a good idea. I didn't remove the requirement of which HTTP methods should be used in JAX-RS participant methods, I've just moved it to the table here. IMO we should take care of #146 in separate PR.

The @Status method returns null if the participant does not know about the LRA. Since a null value or a status are valid why aren't you using java.util.Optional (I believe it was added to the language for this kind of purpose).

This is a good idea. Does it apply only to ParticipantStatus return type or Response too? I would suggest that for Response we keep the status code value 404. That means I would only change return type ParticipantStatus to Optional<ParticipantStatus>. So we will have something like: public CompletionStage<Optional<ParticipantStatus>> status(URI lraId).

Sections 3.2.10 (@Status) and 3.2.11 (@forget) imply that the annotated method should be a JAX-RS endpoint (which is no longer the case).

+1, thanks I missed this

[nicolaferraro]

@nicolaferraro This PR has significant overlap with the current LRAManagment/LRAParticipant API which we would therefore like to remove. Instead we now support compensation via the standard Java annotations: @Compensate and @Forget and, optionally, @Complete and @Status. But note that the method annotated with @LRA still needs to be a JAX-RS endpoint.

We will call @Compensate as usual when the LRA is cancelled. The method must conform to one of the predefined signatures mentioned in this PR which must be validated by an annotation processor when the application is deployed. If you cannot compensate immediately then you can either return an instance of CompletionStage that we will use to monitor your progress, otherwise we will monitor your progress by calling the @Status method. After a crash we will call the status method if it's present otherwise the implementation will re-invoke the compensate method. If you report FailedToCompensate you need to remember that you failed until we call the @Forget method.

Please can you verify that it is okay to remove LRAManagment/LRAParticipant?

To me it is fine to remove it in 1.0. We were not using it currently but we were planning to use it when this spec become final. I think we can skip it in 1.0, also because it missed some of the lifecycle hooks we'd like to use, and thinking to introduce a full programmatic API it in one of next 1.x releases.

[mmusgrov]

@nicolaferraro We won't be removing it and adding it back in again in 1.x - ie I doubt it will be a full programmatic API. You would need use the features provided by this PR, namely to provide a class annotated with @Compensate, @Forget etc. In particular there will be no equivalent of LRAParticipant.

Does this new behaviour still satisfy your requirements?

[mmusgrov]

The annotation for @compensate says that 412 Precondition Failed can be returned. What is the equivalent behaviour for non JAX-RS compensation.

The javadoc for LRA annotations states The annotation <b>SHOULD</b> be applied to JAX-RS annotated methods otherwise it <b>MAY</b> have no effect.. Therefore if implementation allows LRA on non-JAX-RS methods it should be implementation detail how such situations are handled? In every case, we can require to throw InvalidLRAParticipantDefinitionException if such case is detected if you prefer.

I think it would remove the potential for confusion if you update the last paragraph of the javadoc for @Compensate and @Complete to make it clear the comment only applies to JAX-RS resource methods.

The javadoc for @compensate refers the reader to the spec if the signature is invalid. To keep the javadoc self contained it would help to include a javadoc @link to InvalidLRAParticipantDefinitionException which states which signatures are valid. Similarly for @complete, @forget and @Status.

+1

You didn't update the javadoc for InvalidLRAParticipantDefinitionException to include the definition of which signatures qualify as being valid.

What is your opinion about issue #146 (Allow JAX-RS endpoints Complete/Compensate to be invoked using any HTTP method). If we allow that we don't need any javadoc text for it (for example you removed the @delete requirement from the javadoc for @forget and @get from @Status which implies that you also found it a problem having to keep javadoc and spec doc in sync).

I think #146 is a good idea. I didn't remove the requirement of which HTTP methods should be used in JAX-RS participant methods, I've just moved it to the table here. IMO we should take care of #146 in separate PR.

Okay, so would you mind adding a +1 to #146 so that someone can start workng on a PR for it?

The @Status method returns null if the participant does not know about the LRA. Since a null value or a status are valid why aren't you using java.util.Optional (I believe it was added to the language for this kind of purpose).

This is a good idea. Does it apply only to ParticipantStatus return type or Response too? I would suggest that for Response we keep the status code value 404. That means I would only change return type ParticipantStatus to Optional<ParticipantStatus>. So we will have something like: public CompletionStage<Optional<ParticipantStatus>> status(URI lraId).

I think you convinced us in the hangout that using Optional can make the code look clunky so null was preferable.

                                                                                                                                     1,0-1         T

[mmusgrov]

@xstefank I think the Camel team are happy to work with the new API presented in this PR so would you mind also including a commit that removes LRAManagement and LRAParticipant since this PR supercedes its funtionality (probably best not to squash it)

[mmusgrov]

The LRAManagement and LRAParticipant APIs are still present in your last commit. You had given me a +1 to the my comment in the previous review:

I think the Camel team are happy to work with the new API presented in this PR so would you mind also including a commit that removes LRAManagement and LRAParticipant since this PR supercedes its functionality (probably best not to squash it)

Also, the latest commit still has the same text about what happens if an exception is thrown from the status and forget methods:

any other exception results into FailedToComplete or FailedToCompensate participant states

My comment in the previous PR review was:

I think it is valid to infer FailedToCompensate if the compensate method throws an unexpected exception. But if the forget or status methods throw exceptions then the participant may already have compensated or is in the process of compensating and telling the coordinator the wrong thing is either suppressing failure conditions or is reporting false positives, both of which are undesirable.

I do notice that the @Status and @forget methods are under-specified but I would choose to report one of the HTTP 500 codes and leave it up to the implementation to decided what behaviour is preferable, but if it does not retry then it SHOULD log the failure.