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

Remove TimeLimit and TimeUnit on @Complete and @Compensate #233

Closed
rdebusscher opened this issue Sep 5, 2019 · 8 comments · Fixed by #236
Closed

Remove TimeLimit and TimeUnit on @Complete and @Compensate #233

rdebusscher opened this issue Sep 5, 2019 · 8 comments · Fixed by #236
Assignees
Milestone

Comments

@rdebusscher
Copy link
Member

@complete is optional and thus it makes more sense that developer specifies timeout on @compensate.

It also makes more sense When timeLimit is reached, the @Compensate is called (so they have defined the time limit on the method which get called) which is not the case when defined on @complete because the @compensate will be called in that case.

@rdebusscher rdebusscher added this to the 1.0 milestone Sep 5, 2019
@xstefank
Copy link
Member

xstefank commented Sep 5, 2019

+1 good catch

@mmusgrov
Copy link
Contributor

mmusgrov commented Sep 9, 2019

The timeLimit on @Complete indicates how long the participant can guarantee completion.
The timeLimit on @Compensate indicates how long the participant can guarantee compensation.

Therefore the two time limits are distinct and both make sense (they convey important/useful and distinct behaviour) - but if either limit is breached the LRA must cancel.

The time limit does contain semantically meaningful information - suppose the participant can always compensate but clean up actions are time bound. In this case, putting an artificial time limit on compensate is then misleading.

[Aside: having distinct attributes on the compensate and complete endpoints also makes sense if the endpoints are handled by different services - I think we did support this in an early version of the client API]

@rdebusscher
Copy link
Member Author

I can see the reasoning behind it but find it too complicated with too many possibilities which can be simplified.

A timeout for the LRA and a time out for the participant (through @compensate which is required to be a participant).

And if needed for client API (which would complicate the situation again since a participant can join an LRA without the need for any @compensate method which we now declare as required) we can always add it at that moment without breaking compatibility.

@xstefank
Copy link
Member

I was thinking about this and I think we can remove time limits from Complete and Compensate altogether because when there is @Compensate method in the class there must also be an @LRA annotation on different method which can define the participant timeout. In fact, the JavaDoc states:

* If the annotated method runs with an LRA context then this element determines
* the period for which the LRA will remain valid. When this period has
* elapsed the LRA becomes eligible for cancellation.

which I believe is precisely what Complete/Compensate timeouts are doing right now. So the user doesn't need to define timeouts in different places and he/she just needs to choose the lower of value from different Complete/Compensate periods and include it in the @LRA annotation.

So if @LRA starts a new LRA then the time limit is for the duration of the LRA. If @LRA defines join LRA operation, the time limit is still for the duration of the LRA but from the perspective of the participant.

@mmusgrov
Copy link
Contributor

I was thinking about this and I think we can remove time limits from Complete and Compensate altogether because when there is @Compensate method in the class there must also be an @LRA annotation on different method which can define the participant timeout.

This seems reasonable for the annotation based approach (though not for the future client API). We will loose the semantic information I referred to above but your proposal simplifies the spec so I am in favour of your suggestion.

@mmusgrov
Copy link
Contributor

If we all agree with the proposal made by @xstefank does anyone want to take on the issue? If not I will take a look the week after next.

@xstefank
Copy link
Member

I will probably have some time next week if it's in hurry. If not, go ahead, please.

@rdebusscher rdebusscher self-assigned this Sep 12, 2019
@rdebusscher
Copy link
Member Author

I'll have time tomorrow.

I'll update the API and spec to remove TimeLimit and TimeUnit from @compensate and @complete.

@rdebusscher rdebusscher changed the title Remove TimeLimit and TimeUnit on @Complete Remove TimeLimit and TimeUnit on @Complete and @Compensate Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants