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

Provide a way to set a request timeout using annotation #4463

Closed
ikhoon opened this issue Oct 5, 2022 · 4 comments · Fixed by #4499
Closed

Provide a way to set a request timeout using annotation #4463

ikhoon opened this issue Oct 5, 2022 · 4 comments · Fixed by #4499

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Oct 5, 2022

The timeout of an annotated service could be set using AnnotatedServiceBindingBuilder.requestTimeoutMillis().
However, it is globally applied to all methods of the service. If users want to set a custom timeout, they have to call ServiceRequestContext.setRequestTimeout() manually in a service or decorator.

It would be easy to set a timeout of a service or method if we introduce @RequestTimeout. For example:

@RequestTimeout(value = 5, TimeUnit.SECONDS)
class MyService {

    // Override the service timeout and set the timeout to 10 seconds
    @RequestTimeout(value = 10, TimeUnit.SECONDS)
    @Get
    ...
    public CompletableFuture<List<Item>> getItems(...);

    // Limit timeout to 5 seconds
    @Get
    ...
    public CompletableFuture<Item> getItem(...);
}

If both AnnotatedServiceBindingBuilder.requestTimeout() and @RequestTimeout() is set, we can give priority to @RequestTimeout.

@mscheong01
Copy link
Contributor

Would adding a new decorater that does ServiceRequestContext.setRequestTimeout(SET_FROM_START, ), then implementing the new @RequestTimeout with @DecoratorFactory be a viable solution for this issue? Though this would create dependency to the armeria decorator implementation, it would also allow us to use the annotation on other services (such as gRPC services) not only annotated services. If not so, I guess the annotation should be collected and acknowledged when ServiceConfig is created

@ikhoon
Copy link
Contributor Author

ikhoon commented Oct 18, 2022

The decorator approach with setRequestTimeout sounds good to me. 👍

By the way, I came up with this idea while having a conversation with @ks-yim in the LINE internal channel.
Meanwhile, I asked if he would like to implement this feature. So I want to know if @ks-yim is still interested in this. 😀

@mscheong01
Copy link
Contributor

Nice 👏 😄 If not so, you could toss it to me 🙂

@ks-yim
Copy link
Contributor

ks-yim commented Oct 18, 2022

@j-min5u I haven't started working on it. It's all yours, thanks!

minwoox pushed a commit that referenced this issue Dec 9, 2022
Motivation:

Provide a way to set a request timeout using annotation

Modifications:
- Add @interface RequestTimeout
- Add RequestTimeoutDecoratorFunction

Result:

- Closes #4463
- users can use RequestTimeout annotation to set request timeout to service methods
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.

3 participants