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

[Question] - Annotation based Retry After Exception #25

Open
sebeard opened this issue Jul 18, 2019 · 5 comments
Open

[Question] - Annotation based Retry After Exception #25

sebeard opened this issue Jul 18, 2019 · 5 comments
Assignees
Labels
Waiting for feedback Waiting for feedback/some action to be able to move forward

Comments

@sebeard
Copy link

sebeard commented Jul 18, 2019

In your README you give an example of RetryAfterCertainTimeException. How does this work with Feign's RetryableException? I'm trying to implement a RetryAfterException that gets automatically retried, but without duplicating Feign's RetryAfterDecoder (or putting my exception class in the feign.codec package).

@saintf
Copy link
Collaborator

saintf commented Jul 22, 2019

If you take a look at https://github.com/OpenFeign/feign/blob/master/core/src/main/java/feign/SynchronousMethodHandler.java (notice it's a final class/etc) you'll see that at method public Object invoke(Object[] argv) (line 74 at the time of this note) there's an attempt to decode the response/etc. That's where RetryableException gets caught. It then hands the retry to the Retryer (regardless of what Retryer you've configured).

That means that all your exception needs to do to get passed to the Retryer is extend RetryableException - MyAwesomeRetryableException extends RetryableException.

You can then basically implement whatever you want as that exception and override the retryAfter() method to return whatever date you so desire to retry after - wether you choose to use the headers, or just a hard-coded value (timeOfExceptionCreation + 1000 for example) - it's all completely up to you :).

Let me know if that answers your question (and if so - please close ;))

Thanks,
-Fernando

@saintf saintf added the Waiting for feedback Waiting for feedback/some action to be able to move forward label Jul 22, 2019
@saintf saintf self-assigned this Jul 22, 2019
@sebeard
Copy link
Author

sebeard commented Jul 23, 2019

I think so it pretty much ties up what I already had. The problem is that the RetryDecoder in feign.codec.ErrorDecoder only has package local visibility (see https://github.com/OpenFeign/feign/blob/master/core/src/main/java/feign/codec/ErrorDecoder.java). To use this the exception I create would need to move my exception class to feign.codec OR duplicate the RetryDecoder into my own code base, which is a little frustrating.

Here's what I have which works;

package feign.codec;

import feign.RetryableException;
import feign.codec.ErrorDecoder.RetryAfterDecoder;

import java.util.Collection;
import java.util.Map;

import static feign.Util.RETRY_AFTER;

public abstract class RetryAfterException extends RetryableException {

    private static final long serialVersionUID = 1L;

    public RetryAfterException(int code, Map<String, Collection<String>> headers, String body) {
        super(code, body, null, new RetryAfterDecoder().apply(retryAfterOrNull(headers)));
    }

    private static <T> T retryAfterOrNull(Map<String, Collection<T>> map) {
        if (map.containsKey(RETRY_AFTER) && !map.get(RETRY_AFTER).isEmpty()) {
            return map.get(RETRY_AFTER).iterator().next();
        }
        return null;
    }


}

I guess the best solution would be to raise an issue on feign itself to get that access opened up a little more.

Thanks for the support @saintf

@saintf
Copy link
Collaborator

saintf commented Jul 23, 2019

No prob. I'm now confused as to what it was you were trying to achieve... It isn't clear to me.

When do you want to retry and how?

@saintf
Copy link
Collaborator

saintf commented Jul 23, 2019

(hit submit too soon...)

The RetryDecoder is an error decoder (just like the annotation-error-decoder is one). Feign only lets you pick one. I'd argue rather than using the RetryDecoder, you'd use this one to generate an exception that consumes the response and sets the retryAfter.

I think what you're trying to say is that there's some functionality (decoding the header) that is already written in the RetryDecoder that would wouldn't want to duplicate (specifically, reading the header).

I wonder if it makes sense to enhance that functionality to move to either the RetryableException (so that it offers it by consuming the response) or if we perhaps somehow allow for that functionality to exist in the annotation-error-decoder (though that doesn't feel completely like the right place as reading those headers is a more generic task - even if it did offer some extra value for the annotation decoder).

Thoughts?

@hellboy81
Copy link

serialVersionUID should be unique for each exception class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for feedback Waiting for feedback/some action to be able to move forward
Projects
None yet
Development

No branches or pull requests

3 participants