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

Add option to end LRA only at the service that started it #260

Open
xstefank opened this issue Dec 2, 2019 · 8 comments
Open

Add option to end LRA only at the service that started it #260

xstefank opened this issue Dec 2, 2019 · 8 comments
Milestone

Comments

@xstefank
Copy link
Member

xstefank commented Dec 2, 2019

We could add an option to the LRA annotation that would prohibit different services to which the LRA is propagated to close the LRA (and thus it can be invalidated anywhere). In this sense, only when the calls return responses to the initiating service the LRA can be ended and thus it gives a guarantee to the initiating service that the LRA is still active. The name of such a possible parameter is up for discussion.

@xstefank xstefank added this to the 1.x milestone Dec 2, 2019
@mmusgrov
Copy link
Contributor

mmusgrov commented Dec 2, 2019

This is too restrictive. For example if the initiator crashes then it is not possible to close the LRA. It also adds unnecessary coupling between services.

@mmusgrov
Copy link
Contributor

mmusgrov commented Dec 3, 2019

Can we close this issue?

@xstefank
Copy link
Member Author

xstefank commented Dec 3, 2019

This is too restrictive. For example if the initiator crashes then it is not possible to close the LRA. It also adds unnecessary coupling between services.

If any service which is intended to close the LRA (even now if it's closed elsewhere) crashes, it can be restarted so I don't understand this argument. And LRA itself introduces coupling because of the transaction that is propagated so services need to be aware that they are participating in the particular LRA. And after all, this would be optional opt-in on the developer side if there is a use-case for it. So I would prefer to not close it yet unless everyone thinks that it's not a good idea.

@tomjenkinson
Copy link
Contributor

I can't recall the exact details but does the spec allow new participants to be added that can reduce the timelimit of the LRA? If that is the case wouldn't users just register dummy participants with very minimal timelimit to work around any restriction? Even if not it might decide to hold processing indefinitely to trigger the cancellation.

@xstefank
Copy link
Member Author

xstefank commented Dec 3, 2019

@tomjenkinson yes, that is possible now but I don't understand how it relates to this issue. I am proposing the opposite, to disallow other participants to end the LRA (up to the timelimits -> that's a good point).

@rdebusscher
Copy link
Member

If we would allow such restriction, then it needs to be configurable for each LRA separately.

But I'm not sure what should happen if some other participant tries to close the LRA or what happens when there is an 'exception' or status 500 is thrown. Just ignore or close the LRA?

So more details are required, I guess.

@xstefank
Copy link
Member Author

xstefank commented Dec 3, 2019

it would be configurable for each LRA in the LRA annotation. And I agree that this would need to be clarified but I am bringing this up for a discussion now whether or not it is a useful idea.

@tomjenkinson
Copy link
Contributor

@tomjenkinson yes, that is possible now but I don't understand how it relates to this issue. I am proposing the opposite, to disallow other participants to end the LRA (up to the timelimits -> that's a good point).

That was really the main part of my point. That if we impose a way to prevent services closing the LRA (and perhaps I am thinking to the point where we have a client API) then any restriction on that method could be circumvented by a sleep (I expect bad for performance of the system I assume something we do not want to encourage) or affecting the timelimit.

More details will help but no urgency from my side if it can wait to 1.x (which I assume it can as you added that milestone). Thanks

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

No branches or pull requests

4 participants