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

Fault Tolerance in config file could use dash seperated names #24533

Closed
manofthepeace opened this issue Mar 24, 2022 · 6 comments · Fixed by #44831
Closed

Fault Tolerance in config file could use dash seperated names #24533

manofthepeace opened this issue Mar 24, 2022 · 6 comments · Fixed by #44831
Assignees
Milestone

Comments

@manofthepeace
Copy link
Contributor

Description

We have mutliple rest clients in our apps, and we want all retries to be configurable. It works really well in the config file, but the syntax is a little odd, and a little non obvious as well, since we need to know the bean name that is generated from the interface (the $$CDIWrapper portion).

Example

@RegisterRestClient(configKey = "remote-api")
@Retry(maxRetries = 3, jitter = 200, delay = 500)
public interface RemoteApi {}

The config would look like this;

org.acme.rest.RemoteApi$$CDIWrapper/Retry/maxRetries=3
org.acme.rest.RemoteApi$$CDIWrapper/Retry/delay=10000

and with methods;

org.acme.rest.RemoteApi$$CDIWrapper/myMethod/Retry/maxRetries=3
org.acme.rest.RemoteApi$$CDIWrapper/myMethod/Retry/delay=10000

Implementation ideas

Would be nice if a similar idea to what was done to the mp-rest config could apply to SR FT. Like using the config-key and using dashes instead of slashes.

Example
quarkus.faulttolerance.[configkey or class].[method if any].retry.max-retries=3

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 24, 2022

/cc @Ladicek

@Ladicek Ladicek self-assigned this Mar 24, 2022
@Ladicek
Copy link
Contributor

Ladicek commented Mar 24, 2022

That makes sense. I'm assigning this to myself, but I'm pretty sure I won't get to this anytime soon, so if someone else wants to tackle it, that's fine too :-)

@mikethecalamity
Copy link

I think this is a great idea

@Ladicek
Copy link
Contributor

Ladicek commented Nov 26, 2024

For the record, I'm working on this now, but I won't satisfy all requests from this issue. In particular, there shall be no support for the RestClient config key in Fault Tolerance configuration, and there will be no automatic detection / removal / support / whatever of the $$CDIWrapper suffix. That's an implementation decision of RestClient that has nothing to do with Fault Tolerance.

@gsmet
Copy link
Member

gsmet commented Nov 26, 2024

@Ladicek while I kinda agree with you, we are supposed to have a cohesive platform and having to rely on internal implementation details to configure a component looks less than ideal.

In HV, I know we have an SPI to allow for some unwrapping. I'm not sure what the solution would be in this case but the current situation is less than ideal from a user POV.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 26, 2024

Well yes, I agree. I admit I didn't spend too much time thinking about it, but I don't see a good solution at the moment. (No way I'm adding a special case to SmallRye Fault Tolerance just for RestClient Reactive, that's ugly and super brittle.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants