quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.87k stars 2.71k forks source link

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

Closed manofthepeace closed 1 day ago

manofthepeace commented 2 years ago

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[bot] commented 2 years ago

/cc @Ladicek

Ladicek commented 2 years ago

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 commented 1 year ago

I think this is a great idea

Ladicek commented 5 days ago

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 commented 4 days ago

@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 commented 4 days ago

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.)