Prototype "before retry action" support

[hbelmiro]

PR to solve #259.

Considerations:

• This is just an initial implementation. I decided to create this PR to get feedback and discover if it fits with the expectation.
• I implemented the minimum possible to get the feedback fast.
• The implementation is based on Fallback.
• I created io.smallrye.faulttolerance.api.BeforeRetryAnnotation. I don't know if this feature will be specific to SmallRye of if it's intended to be added to MicroProfile. I provisionally put the suffix 'Annotation' in its name to not confuse with the implementation class for now.
• I only implemented the attribute beforeRetryMethod. It should be used like the following:

    @Retry(maxRetries = 2)
    @BeforeRetryAnnotation(beforeRetryMethod = "beforeRetry")
    public String call() {
        if (attempt.getAndIncrement() < 1) {
            throw new IllegalStateException();
        }
        return "call " + attempt.get();
    }

    public void beforeRetry() {
        beforeRetryRuns.incrementAndGet();
    }

• I ignored all the async logic for now
• I created a very simple test io.smallrye.faulttolerance.before.retry.BeforeRetryTestBean and it's working.

[Ladicek]

Hi, sorry for not getting to this sooner, I'm quite busy elsewhere. I didn't really expect anyone to pick up ideas from the backlog and run with them :-) For next time, I'd probably recommend trying to start a discussion on the issue -- though of course a preliminary PR like this can be seen as a form of starting a discussion. I converted this PR to a draft to highlight that.

I confirm that the idea was to make this a SmallRye-specific feature for now. The code looks mostly fine, though there is one thing that I didn't expect. You implement "before retry" support as an additional FT strategy, which is inserted into the chain immediately after the "retry" strategy -- while I originally planned to embed this directly into the "retry" strategies. Clearly it works, though I'm not sure if it perhaps prevents some interesting features (such as aborting the entire retry) and if it maybe interacts weirdly with exception handling. Those are API design questions, though. Actually API design is at least as important as implementation, if not more -- but unfortunately, you left the annotation undocumented. That would have to change for sure, before this could be merged.

Also I think the core implementation should be directly in the retry package. And the API annotation should be marked @Experimental, like other parts of the API. And FaultToleranceOperation should validate that @BeforeRetry only appears on methods that also have a @Retry.

[hbelmiro]

Hi @Ladicek.
Thank you very much for your feedback.

Hi, sorry for not getting to this sooner, I'm quite busy elsewhere. I didn't really expect anyone to pick up ideas from the backlog and run with them :-) For next time, I'd probably recommend trying to start a discussion on the issue -- though of course a preliminary PR like this can be seen as a form of starting a discussion. I converted this PR to a draft to highlight that.

No problem. I created the PR to start the discussion (and see if I would be able to implement this) :-)

I confirm that the idea was to make this a SmallRye-specific feature for now. The code looks mostly fine, though there is one thing that I didn't expect. You implement "before retry" support as an additional FT strategy, which is inserted into the chain immediately after the "retry" strategy -- while I originally planned to embed this directly into the "retry" strategies. Clearly it works, though I'm not sure if it perhaps prevents some interesting features (such as aborting the entire retry) and if it maybe interacts weirdly with exception handling. Those are API design questions, though. Actually API design is at least as important as implementation, if not more -- but unfortunately, you left the annotation undocumented. That would have to change for sure, before this could be merged.

"That would have to change for sure, before this could be merged."
It's not clear to me what has to be changed. Is the fact that "before retry" is implemented as an additional FT strategy or the lack of documentation? - or both? :-)
I agree that may be better/less dangerous to embed into the "retry" strategy.
I left the annotation undocumented because I wanted to create an initial PR and get your feedback ASAP.

Also I think the core implementation should be directly in the retry package. And the API annotation should be marked @Experimental, like other parts of the API. And FaultToleranceOperation should validate that @BeforeRetry only appears on methods that also have a @Retry.

👍🏽 I'll do that.

I might not be able to touch on this in the next few of days, but since you're busy with other stuff, that might not be a problem.

[Ladicek]

"That would have to change for sure, before this could be merged."
It's not clear to me what has to be changed. Is the fact that "before retry" is implemented as an additional FT strategy or the lack of documentation? - or both? :-)

Documentation is what I meant here.

The implementation strategy is certainly valid -- I just didn't expect it, that's all. When you (or me, or someone else) write the documentation, it might turn out that this implementation strategy perhaps isn't the best, or maybe it is, I don't know :-)

I started prototyping another feature (rate limiting) and API with documentation is the first thing I did.

I might not be able to touch on this in the next few of days, but since you're busy with other stuff, that might not be a problem.

That is not a problem at all!

[hbelmiro]

I'm closing this PR since I won't be able to work on this in the short term.

[Ladicek]

No worries, and thanks for spending your time on this!