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

Per-service setting is not possible for annotated service #2023

Closed
minwoox opened this issue Aug 23, 2019 · 6 comments · Fixed by #2222
Closed

Per-service setting is not possible for annotated service #2023

minwoox opened this issue Aug 23, 2019 · 6 comments · Fixed by #2222
Labels

Comments

@minwoox
Copy link
Member

minwoox commented Aug 23, 2019

A user can configure per-service settings using fluent Route API.
However, this is only for the services which implement HttpService, not for the annotated service.
We should provide a way for annotated service as well.

@minwoox minwoox added the defect label Aug 23, 2019
@sivaalli
Copy link
Contributor

sivaalli commented Oct 3, 2019

Hi @trustin @minwoox @ikhoon ,

I think what we could do is, add an annotation called @Configuration that has properties/fields that reflect all the settings that Route api supports. This annotation will be used on each service method. For example a user can declare @Configuration(maxRequestLength = 1024, requestTimeoutMillis=5000) like this on a service method. Instead of creating a new Annotation(in this example @Configuration), we can also add route specific configuration properties on @Path. What do you think?

If you guys have anything else in mind please feel free to share it so I can implement it.

@trustin
Copy link
Member

trustin commented Oct 4, 2019

How about just adding buildAnnotated(Object) to ServiceBindingBuilder?

@minwoox
Copy link
Member Author

minwoox commented Oct 4, 2019

Hello, @sivaalli! Thanks for the suggestion. However, I think adding more annotations is the last thing I want to do. 😄

Had a chat with @trustin and we thought adding a builder would be enough for this.

ServerBuilder sb = Server.builder()          // Returns ServerBuilder
                         .annotatedService() // Returns AnnotatedServiceBindingBuilder
                                             // Not sure this is the best name.
                         .requestTimeoutMillis(5000)
                         .contentPreview(...)
                         ...
                         .build(object);     // Returns to the ServerBuilder

All the properties that a service might want to set separately are gathered in AbstractServiceBindingBuilder thanks to @ikhoon.
However, the builder for annotated service just cannot inherit from it because of AbstractBindingBuilder which AbstractServiceBindingBuilder is inherited from.

- AbstractBindingBuilder
  - AbstractServiceBindingBuilder
    - ServiceBindingBuilder

AbstractBindingBuilder has setters related to paths and HTTP methods which an annotated service does not need.
So I think we have to tear off AbstractServiceBindingBuilder from AbstractBindingBuilder first. We can make an interface for the service configuration binding, so ServiceBindingBuilder implements the interface while extending AbstractBindingBuilder, and AnnotatedServiceBindingBuilder(?) will implements the interface only.
What do you think?

@sivaalli
Copy link
Contributor

sivaalli commented Oct 4, 2019

Hi @trustin and @minwoox, I think I may have misunderstood this issue. So just want to reiterate the idea. For example, if this is my service:

class MyService{

    @Get("/foo")
    public HttpResponse foo(){
        return HttpResponse.of(200);
    }

    @Post("/bar")
    public HttpResponse bar(){
        return HttpResponse.of(200);
    }
}

and annotated service is registered using

Server server = new ServerBuilder().port(8080, SessionProtocol.HTTP).annotatedService("/hello", new MyService()).build();

Then, we should provide ability for user to

  1. Configure different http settings for /hello/foo(for ex: requestTimeoutMillis =1000) and /hello/bar(for ex: requestTimeoutMillis =5000)
  2. Configure http settings for anything under /hello

If I'm understanding this correct the point# 2 is what we are trying to achieve right?

But your idea sounds good. Sorry If I'm saying things again and again but just wanted to make sure I understand this correctly before I start writing any code.

@minwoox
Copy link
Member Author

minwoox commented Oct 4, 2019

If I'm understanding this correct the point# 2 is what we are trying to achieve right?

Yes, we are going to provide #2 because this is per-service configuration not per-method configuration. :-)

Configure different http settings for /hello/foo(for ex: requestTimeoutMillis =1000) and /hello/bar(for ex: requestTimeoutMillis =5000)

In this case, we are guiding like:

class MyService{
    @Get("/foo")
    public HttpResponse foo(ServiceRequestContext ctx) { // or RequestContext.current()
        ctx.setRequestTimeoutMillis(1000);
        return HttpResponse.of(200);
    }
}

Sorry If I'm saying things again and again but just wanted to make sure I understand this correctly before I start writing any code.

You are always welcomed to ask us anything. :-)

@sivaalli
Copy link
Contributor

sivaalli commented Oct 4, 2019

Thank you @minwoox. I will start looking into this.

trustin pushed a commit that referenced this issue Nov 4, 2019
Motivation:

It is good to provide an ability to user to configure per service level settings for annotated service.

Modifications:

- Pulled all methods that `AbstractServiceBindingBuilder` (minus `AbstractBindingBuilder`) has into an interface called `ServiceConfigSetter`.
- `AbstractServiceBindingBuilder` now implements this (`ServiceConfigSetter`) interface and also extends `AbstractBindingBuilder`.
- Added a default implementation of `ServiceConfigSetter` called `DefaultServiceConfigSetter`, which is shared between `AbstractServiceBindingBuilder` and `AbstractServiceBuilder` to avoid code duplication.
- Added `AnnotatedServiceBindingBuilder` and `VirtualHostAnnotatedServiceBindingBuilder`.

Result:

- Closes #2023
- Annotated services can now be configured to have service-specific properties.
eugene70 pushed a commit to eugene70/armeria that referenced this issue Nov 10, 2019
Motivation:

It is good to provide an ability to user to configure per service level settings for annotated service.

Modifications:

- Pulled all methods that `AbstractServiceBindingBuilder` (minus `AbstractBindingBuilder`) has into an interface called `ServiceConfigSetter`.
- `AbstractServiceBindingBuilder` now implements this (`ServiceConfigSetter`) interface and also extends `AbstractBindingBuilder`.
- Added a default implementation of `ServiceConfigSetter` called `DefaultServiceConfigSetter`, which is shared between `AbstractServiceBindingBuilder` and `AbstractServiceBuilder` to avoid code duplication.
- Added `AnnotatedServiceBindingBuilder` and `VirtualHostAnnotatedServiceBindingBuilder`.

Result:

- Closes line#2023
- Annotated services can now be configured to have service-specific properties.
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
Motivation:

It is good to provide an ability to user to configure per service level settings for annotated service.

Modifications:

- Pulled all methods that `AbstractServiceBindingBuilder` (minus `AbstractBindingBuilder`) has into an interface called `ServiceConfigSetter`.
- `AbstractServiceBindingBuilder` now implements this (`ServiceConfigSetter`) interface and also extends `AbstractBindingBuilder`.
- Added a default implementation of `ServiceConfigSetter` called `DefaultServiceConfigSetter`, which is shared between `AbstractServiceBindingBuilder` and `AbstractServiceBuilder` to avoid code duplication.
- Added `AnnotatedServiceBindingBuilder` and `VirtualHostAnnotatedServiceBindingBuilder`.

Result:

- Closes line#2023
- Annotated services can now be configured to have service-specific properties.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment