Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflict in TCK: unable to cancel already closed nested LRA #274

Closed
xstefank opened this issue Feb 1, 2020 · 17 comments · Fixed by #293
Closed

Conflict in TCK: unable to cancel already closed nested LRA #274

xstefank opened this issue Feb 1, 2020 · 17 comments · Fixed by #293
Assignees
Labels
PR sent The issue is waiting for its associated PR to be merged
Milestone

Comments

@xstefank
Copy link
Member

xstefank commented Feb 1, 2020

With #244 we require now that every LRA resource must contain either Compensate or AfterLRA method:

  • Compensate - requires that received/created LRA is active when the LRA method is invoked
  • AfterLRA - requires that received/created LRA is active, Closing, or Cancelling when the LRA method is invoked

In the TCK test TckTests#mixedMultiLevelNestedActivity we test the scenario where the nested LRA is first closed and then subsequently canceled. That means that we are invoking LRA method with already closed LRA which together with the requirement of #244 means that such invocations will always fail.

This means that #244 basically removed the possibility of mixed nested outcomes from 1.0. It will be possible to simulate TckTests#mixedMultiLevelNestedActivity scenario with the LRAClient when it will be introduced later.

For now, I added ignore to TckTests#mixedMultiLevelNestedActivity in PR #273.

@xstefank
Copy link
Member Author

xstefank commented Feb 1, 2020

Possible solutions (I assume we can't change the semantics of Compensate methods):

  • remove the requirement from AfterLRA resource on particular LRA status when LRA method is invoked (if LRA is already closed and implementation doesn't know about it anymore, the AfterLRA method will never be invoked)

  • update the specification text about mixed nested outcomes (if there are any mentions) and remove TckTests#mixedMultiLevelNestedActivity (we can reintroduce this test when we add LRAClient interface

@rdebusscher
Copy link
Member

  • Compensate - requires that received/created LRA is active when the LRA method is invoked
  • AfterLRA - requires that received/created LRA is active, Closing, or Cancelling when the LRA method is invoked

This is about when the LRA method is executed, not about when the @compensate annotated method is executed.

I also doesn't see any change in #265 which indicates that something has changed regarding when the @compensate annotated method can be executed.

In the section of Forgetting an LRA there is the explicit mention that @compensate can be called again when the parent LRA closes

a nested LRA can be closed independently from its parent but
it must retain the ability to compensate until the parent has finished.

So I for me this issue is invalid.

@xstefank
Copy link
Member Author

xstefank commented Feb 2, 2020

@rdebusscher what do you mean, please? In the copied statement it is said that we are talking about LRA methods (e.g., Compensate - requires that received/created LRA is active when the LRA method is invoked).

The issue is that in the test the LRA is closed by the user and then canceled by the user. So when the cancel is invoked, it is invoked with already closed (thus ended) LRA id which means that in both Compensate and AfterLRA participant cases the execution will be rejected as participant cannot enlist with closed LRA id.

This issue is not about Complete or AfterLRA calls per se. It is about the calls to LRA methods where we now require that all LRA resources must be participants or afterLRA listeners.

@rdebusscher
Copy link
Member

@xstefank

Correct, my mistake. I see it also now that nested LRA is immediately closed when called. I was mixing it up with another test that verifies if the nested @compensate is called when parent is cancelled.

We could define another nested LRA which can be cancelled, but I do not see the intention of the test (what does it test in relation to the spec?). So maybe we can remove the test.

@xstefank
Copy link
Member Author

xstefank commented Feb 3, 2020

@mmusgrov can explain more what the test is needed for. But from my understanding this the only test that verifies that nested LRA can be canceled even after it was previously closed.

Also as I mentioned, it will be possible to implement this case when we introduce a non-participating party to the spec (e.g., LRAClient API). But of course, we can remove the test now and add it again when it will be possible to implement it.

@rdebusscher
Copy link
Member

To my understanding, a nested LRA can be cancelled (the @compensate is called) after it had been closed because the parent is cancelled. Not because the nested is cancelled explicitly after it is closed since the nested LRA itself is no longer active.

@xstefank
Copy link
Member Author

xstefank commented Feb 3, 2020

I wasn't able to find a mention of the user cancel of nested LRA in the specification document. Only as @rdebusscher described. @mmusgrov can the test be removed or am I missing something?

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 3, 2020

I wasn't able to find a mention of the user cancel of nested LRA in the specification document. Only as @rdebusscher described. @mmusgrov can the test be removed or am I missing something?

The test is valid. The spec says:

"A nested LRA which has closed must retain the ability to cancel the effects if the the parent cancels"

So in the TCK test we close the nested LRA. The nested participant must retain the ability to cancel the effects (ie the @compensate method if invoked must reverse the effects even though the nested LRA has closed). The nested participant must continue to provide this guarantee until it has been told to forget about the nested LRA. In other words those other issues you reference do not invalidate this requirement.

@xstefank
Copy link
Member Author

xstefank commented Feb 4, 2020

@mmusgrov the test tries to cancel this closed nested LRA as a user manually. Not by canceling parent LRA. So even if nested the sequence of events is:

1/ start LRA with id
2/ close LRA with id (id is closed now)
3/ cancel LRA with id

All these operations performed by the user.

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 4, 2020

So you mean that the test (https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTests.java#L180) starts the parent LRA and invokes the resource lraresource/multiLevelNestedActivity (https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/LraResource.java#L362). The body of this method invokes another resource on line 375 (https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/LraResource.java#L375). The invoked resource (https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/participant/api/LraResource.java#L349) starts a nested LRA and ends it when the resource method finished (because of the annotation @LRA(value = LRA.Type.NESTED, end = true).

The test method then manually cancels the nested LRAs (https://github.com/eclipse/microprofile-lra/blob/master/tck/src/main/java/org/eclipse/microprofile/lra/tck/TckTests.java#L647).

Okay, I see why that is an issue. But we should still test the mixed scenario where one of the nested participants is closed and the other one is cancelled, ie don't delete the test just tweak it.

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 4, 2020

Okay, I see why that is an issue. But we should still test the mixed scenario where one of the nested participants is closed and the other one is cancelled, ie don't delete the test just tweak it.

Actually I spoke too soon. The test is valid since we must allow closed nested LRAs to be cancelled (it is part of the structuring mechanisms that our spec facilitates).

The correct fix is update the section where we discuss validation of LRAs and add the caveat for nested LRAs.

@xstefank
Copy link
Member Author

xstefank commented Feb 4, 2020

@mmusgrov so for nested LRA we allow LRA execution with inactive LRA id but there will be no enlistment?

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 4, 2020

The nested LRA will not be in the ACTIVE state and since our state model only allows enlistment with active LRAs then enlistment should fail.

But if a resource is invoked with a CLOSED nested LRA and if the resource is not already enlisted then we return an error code. But if it is already enlisted then we allow the business method to proceed since the resource will be told to compensate if the LRA is later cancelled. The only issue with this approach is that there would be no @Complete notification (unless we automatically call it when the method finishes). Thoughts?

@xstefank
Copy link
Member Author

xstefank commented Feb 4, 2020

But if it is already enlisted then we allow the business method to proceed since the resource will be told to compensate if the LRA is later cancelled.

What if the LRA business method closes the nested LRA again?

The only issue with this approach is that there would be no @complete notification (unless we automatically call it when the method finishes).

Where there would be no @Complete call? If the nested LRA is closed then there should be Complete call. When it is then later canceled then there should be a Compensate call. If it's later closed again - that is my previous question.

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 5, 2020

But if it is already enlisted then we allow the business method to proceed since the resource will be told to compensate if the LRA is later cancelled.

What if the LRA business method closes the nested LRA again?

Why would that cause a problem?

The only issue with this approach is that there would be no @complete notification (unless we automatically call it when the method finishes).

Where there would be no @Complete call?

On the resource representing the nested participant.

If the nested LRA is closed then there should be Complete call. When it is then later canceled then there should be a Compensate call. If it's later closed again - that is my previous question.

It is not possible to close a cancelled nested LRA. See the javadoc for the LRA annotation nested element.

@mmusgrov
Copy link
Contributor

mmusgrov commented Feb 5, 2020

So further to my last comment: we need to allow the cancellation of closed nested LRAs in order to facilitate the structuring of business work and we can still support it via annotations, with a change to the existing validation logic, as follows:

  • if the implementation knows that the incoming context is nested and not cancelled then we allow enlistment (even if it is closed), otherwise the method is not allowed to proceed.

When the method finishes without error the complete method, if present, would be invoked as normal.

This will work since the @Complete method for nested LRAs is advisory and the business logic must be prepared to be asked to compensate.

@mmusgrov mmusgrov self-assigned this Feb 12, 2020
@mmusgrov mmusgrov added the PR sent The issue is waiting for its associated PR to be merged label Feb 12, 2020
@mmusgrov
Copy link
Contributor

PR #276 was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR sent The issue is waiting for its associated PR to be merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants