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

Adding AnnotatedServiceBuilder to add service level settings to annotated service #2180

Closed
wants to merge 57 commits into from

Conversation

@sivaalli
Copy link
Contributor

sivaalli commented Oct 10, 2019

Motivation:

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

Closes: #2023

Modifications:

  1. Pulled all methods that AbstractServiceBindingBuilder(minus AbstractBindingBuilder) has into an interface called ServiceBuilder
  2. AbstractServiceBindingBuilder implements this(ServiceBuilder) interface and also extends AbstractBindingBuilder
  3. Provided a default implementation of ServiceBuilder called DefaultServiceBuilder, this is shared between AbstractServiceBindingBuilder and AbstractServiceBuilder to avoid code duplication

Result:
AnnotatedService can now be configured to have service specific properties.

@sivaalli sivaalli requested review from ikhoon, minwoox and trustin as code owners Oct 10, 2019
}

public ServiceConfigBuilder serviceConfigBuilder(Route route, Service<HttpRequest, HttpResponse> service) {
final ServiceConfigBuilder serviceConfigBuilder = new ServiceConfigBuilder(route, service);

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 10, 2019

Author Contributor

I could decorate a service with the decorator here, but AnnotatedServiceBindingBuilder needs exceptionHandlingDecorators as the last decorator. Otherwise it is safe and clear to decorate service with decorator here.

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

I see. How about adding that as a Javadoc of this method?

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #2180 into master will decrease coverage by <.01%.
The diff coverage is 77.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2180      +/-   ##
============================================
- Coverage     73.59%   73.58%   -0.01%     
- Complexity     9589     9619      +30     
============================================
  Files           838      841       +3     
  Lines         36944    37053     +109     
  Branches       4554     4555       +1     
============================================
+ Hits          27190    27267      +77     
- Misses         7429     7463      +34     
+ Partials       2325     2323       -2
Impacted Files Coverage Δ Complexity Δ
...ava/com/linecorp/armeria/server/ServerBuilder.java 74.6% <100%> (+0.06%) 107 <1> (+1) ⬆️
...p/armeria/server/AbstractServiceConfigSetters.java 70.96% <70.96%> (ø) 10 <10> (?)
...armeria/server/AnnotatedServiceBindingBuilder.java 76% <76%> (ø) 13 <13> (?)
...rp/armeria/server/DefaultServiceConfigSetters.java 81.81% <81.81%> (ø) 18 <18> (?)
.../armeria/server/AbstractServiceBindingBuilder.java 78.12% <85.71%> (-5.55%) 11 <8> (-8)
.../linecorp/armeria/internal/Http2ObjectEncoder.java 71.42% <0%> (-8.58%) 12% <0%> (-1%)
...meria/internal/AbstractHttp2ConnectionHandler.java 93.33% <0%> (-3.34%) 13% <0%> (-1%)
...a/com/linecorp/armeria/common/util/Exceptions.java 36.84% <0%> (-2.64%) 23% <0%> (-2%)
...om/linecorp/armeria/client/HttpSessionHandler.java 58.47% <0%> (-2.55%) 28% <0%> (-1%)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cca8e3c...ecd8aed. Read the comment docs.

@minwoox minwoox added this to the 0.95.0 milestone Oct 11, 2019
@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 14, 2019

Thanks a lot, @sivaalli, for your work!
Your PR is mostly looking good except for the names of the classes. As you know we have so many classes whose names are similar such as ServiceConfig, ServiceConfigBuilder, ServiceBindingBuilder, AbstractBindingBuilder, AbstractServiceBuilder, AbstractServiceBindingBuilder and so on. This is not the problem you introduced, but we used to have from the past. So I think we need to clean that first.
@trustin, @ikhoon and I will discuss this naming and let you know how we can improve this situation.
Thanks a lot again and sorry for the late review. 🙇

@sivaalli

This comment has been minimized.

Copy link
Contributor Author

sivaalli commented Oct 14, 2019

Thanks a lot, @sivaalli, for your work!
Your PR is mostly looking good except for the names of the classes. As you know we have so many classes whose names are similar such as ServiceConfig, ServiceConfigBuilder, ServiceBindingBuilder, AbstractBindingBuilder, AbstractServiceBuilder, AbstractServiceBindingBuilder and so on. This is not the problem you introduced, but we used to have from the past. So I think we need to clean that first.
@trustin, @ikhoon and I will discuss this naming and let you know how we can improve this situation.
Thanks a lot again and sorry for the late review. 🙇

No problem at all :)

@sivaalli

This comment has been minimized.

Copy link
Contributor Author

sivaalli commented Oct 18, 2019

@trustin, thanks for your review. I have made requested changes. Please take a look :)

}

public ServiceConfigBuilder serviceConfigBuilder(Route route, Service<HttpRequest, HttpResponse> service) {
final ServiceConfigBuilder serviceConfigBuilder = new ServiceConfigBuilder(route, service);

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

I see. How about adding that as a Javadoc of this method?

*/
<T extends Service<HttpRequest, HttpResponse>, R extends Service<HttpRequest, HttpResponse>>
ServiceBuilder decorator(Function<T, R> decorator);

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

nit: This empty line could be removed

@@ -0,0 +1,123 @@
package com.linecorp.armeria.server;

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

If you rebase your branch against master you'll start to see many checkstyle violations, like missing license headers.

* A builder that implements {@link ServerBuilder} by delegating all calls
* to {@link DefaultServiceBuilder}
*/
abstract class AbstractServiceBuilder implements ServiceBuilder {

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

Could you explain why we need this class? It seems to me that we can just extend DefaultServiceBuilder and remove this class, no?

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 21, 2019

Author Contributor

There is no much difference except AbstractServiceConfigSetters(which was AbstractServiceBuilder) provides build0() that builds ServiceConfigBuilder and calls the abstract method serviceConfigBuilder for sub-classes to do something with ServiceConfigBuilder.

AnnotatedServiceBindingBuilder's buildAnnotated will look like this:

final ServiceConfigBuilder serviceConfigBuilder = toServiceConfigBuilder(e.route(), s);
serverBuilder.serviceConfigBuilder(serviceConfigBuilder);

instead of

build0(e.route(), s);

I can remove this class if this class is accomplishing little/no value.

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

I can remove this class if this class is accomplishing little/no value.

Then, let's remove this class and use DefaultServiceConfigSetters. :-)



Comment on lines 1045 to 1039

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

nit: Double empty lines (could be a single line)

return service.decorate(decorator);
}

ServiceConfigBuilder serviceConfigBuilder(Route route, Service<HttpRequest, HttpResponse> service) {

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

toServiceConfigBuilder?

import com.linecorp.armeria.common.logging.ContentPreviewerFactory;
import com.linecorp.armeria.server.logging.AccessLogWriter;

interface ServiceBuilder {

This comment has been minimized.

Copy link
@trustin

trustin Oct 19, 2019

Member

How about renaming ServiceBuilder to ServiceConfigSetters and DefaultServiceBuilder to DefaultServiceConfigSetters? WDYT, @minwoox @ikhoon ?

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 20, 2019

Contributor

That's an excellent idea! I love that name. :-) This class only used for configuring service.

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 19, 2019

This is not the problem you introduced, but we used to have from the past. So I think we need to clean that first.

How about just renaming *ServiceBuilder in this pull request to move forward? I guess we can clean up by ourselves after this is merged. For example, we could extract some common interface & logic between *ServiceBuilder and ServiceConfigBuilder, but I guess it will complicate the progress of #2023.

Copy link
Contributor

heowc left a comment

Good! 👍

import com.linecorp.armeria.server.logging.AccessLogWriter;
import com.linecorp.armeria.testing.junit.server.ServerExtension;

public class AnnotatedServiceBindingBuilderTest {

This comment was marked as resolved.

Copy link
@heowc

heowc Oct 19, 2019

Contributor

It doesn't seem to need to be public


public class AnnotatedServiceBindingBuilderTest {

public static final ExceptionHandlerFunction handlerFunction = (ctx, req, cause) -> HttpResponse.of(501);

This comment was marked as resolved.

Copy link
@heowc

heowc Oct 19, 2019

Contributor

Wouldn't private be better than public?

@sivaalli sivaalli force-pushed the sivaalli:master branch from 3b352b9 to bc8f636 Oct 21, 2019
@sivaalli

This comment has been minimized.

Copy link
Contributor Author

sivaalli commented Oct 21, 2019

@trustin @minwoox @ikhoon @heowc Implemented all the requested changed. Let me know what you think.

Copy link
Member

minwoox left a comment

mostly nits. 😄
Thanks @sivaalli!

*
* @param requestTimeout the timeout. {@code 0} disables the timeout.
*/
abstract class AbstractServiceBindingBuilder extends AbstractBindingBuilder implements

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

It'll be a lot much prettier if we break the line before implements.

}
service = defaultServiceConfigSetters.decorate(service);
final ServiceConfigBuilder serviceConfigBuilder = defaultServiceConfigSetters
.toServiceConfigBuilder(route, service);

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

nit: indentation

* {@code /}
* @return {@link ServerBuilder} to continue building {@link Server}
*/
public ServerBuilder buildAnnotated(Object service) {

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

How about build() as we do in ServiceBindingBuilder?

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 22, 2019

Author Contributor

OK. I used this name because it would be clear to distinguish building annotated service and building server. I can make this change.

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 22, 2019

Author Contributor

For ex, it would look like:

  ServerBuilder sb = Server.builder();
  sb.annotatedService()                                    // Returns an instance of this class
    .requestTimeout(5000)
    .maxRequestLength(8192)
    .exceptionHandler((ctx, request, cause) -> HttpResponse.of(400))
    .pathPrefix("/foo")
    .verboseResponses(true)
    .contentPreview(500)
    .build(new Service())                         // Return to the ServerBuilder.
    .build();

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 23, 2019

Member

Yes, I have thought about it which build(..) is called twice consecutively.
Currently, we do this for route():

Server.builder()
      .route()
      .get("/")
      .build((ctx, req) -> HttpResponse.of(200))
      .build();

If we rename buildAnnotated, it will be:

Server.builder()
      .annotatedService()
      .build(new MyObject())
      .build();

Which is a little weird. 😄
But there's another chance that a user uses it with the variable:

final AnnotatedServiceBindingBuilder annotatedBuilder = Server.builder().annotatedService();
annotatedBuilder.build(new MyObject());

I'm not sure which one is better honestly. 😄 But I still believe that we need to use same method name for the build() in route(). WDYT?

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 23, 2019

Author Contributor

Thanks for the explanation. This totally makes sense. I will make this change along with adding VirtualHostAnnotatedServiceBindingBuilder and will notify you again for review.

/**
* Returns a {@link AnnotatedServiceBindingBuilder} to build annotated service.
*/
public AnnotatedServiceBindingBuilder annotatedService() {

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

Could you add this method to VirtualHostBuilder as well?

This comment has been minimized.

Copy link
@sivaalli

sivaalli Oct 22, 2019

Author Contributor

Sure will do.

*
* @see ServiceBindingBuilder
*/
public final class AnnotatedServiceBindingBuilder extends AbstractServiceConfigSetters {

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

Could you make another class for VirtualHostBuilder? VirtualHostAnnotatedServiceBindingBuilder.

* @param service annotated service object to handle incoming requests matching path prefix, which
* can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}.
Comment on lines 179 to 180

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

nit indentation?

Suggested change
* can be configured through {@link AnnotatedServiceBindingBuilder#pathPrefix(String)}.
* @param service annotated ...
* can be ...
/**
* Adds the given {@link RequestConverterFunction} to this {@link AnnotatedServiceBindingBuilder}.
*/
public AnnotatedServiceBindingBuilder requestHandler(RequestConverterFunction requestConverterFunction) {

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

We use the word converter for the request and response. Could you rename it? requestConverter

@@ -1000,6 +1000,13 @@ public ServerBuilder annotatedService(String pathPrefix, Object service,
return this;
}

/**
* Returns a {@link AnnotatedServiceBindingBuilder} to build annotated service.

This comment has been minimized.

Copy link
@minwoox

minwoox Oct 22, 2019

Member

nit: a -> an

Copy link
Member

trustin left a comment

Excellent job, @sivaalli! Thanks a lot for your patience. 🙏
LGTM once @minwoox's comments are addressed.

service = defaultServiceConfigSetters.decorate(service);
final ServiceConfigBuilder serviceConfigBuilder = defaultServiceConfigSetters
.toServiceConfigBuilder(route, service);
Comment on lines 121 to 123

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 22, 2019

Contributor

I think we need a new local variable here to avoid decorating a service multiple times.

final Service<HttpRequest, HttpResponse> decoratedService = defaultServiceConfigSetters.decorate(service);
final ServiceConfigBuilder serviceConfigBuilder = defaultServiceConfigSetters                                                                    
        .toServiceConfigBuilder(route, decoratedService);
* @return {@link ServerBuilder} to continue building {@link Server}
*/
public ServerBuilder buildAnnotated(Object service) {
final ImmutableList<ExceptionHandlerFunction> exceptionHandlerFunctions =

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 22, 2019

Contributor

Could you change this ImmutableList to List and the following assignments?

Suggested change
final ImmutableList<ExceptionHandlerFunction> exceptionHandlerFunctions =
final List<ExceptionHandlerFunction> exceptionHandlerFunctions =
requestConverterFunctions,
responseConverterFunctions);
final List<AnnotatedHttpServiceElement> elements =
AnnotatedHttpServiceFactory.find(pathPrefix, service, exceptionHandlersAndConverters);

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 22, 2019

Contributor

How about adding a new AnnotatedHttpServiceFactory.find method because the find method splits the merged list(exceptionHandlersAndConverters) again. It seems inefficient.

for (final Object o : exceptionHandlersAndConverters) {
boolean added = false;
if (o instanceof ExceptionHandlerFunction) {
if (exceptionHandlers == null) {
exceptionHandlers = ImmutableList.builder();
}
exceptionHandlers.add((ExceptionHandlerFunction) o);
added = true;
}
if (o instanceof RequestConverterFunction) {
if (requestConverters == null) {
requestConverters = ImmutableList.builder();
}
requestConverters.add((RequestConverterFunction) o);
added = true;
}
if (o instanceof ResponseConverterFunction) {
if (responseConverters == null) {
responseConverters = ImmutableList.builder();
}
responseConverters.add((ResponseConverterFunction) o);
added = true;
}

public static List<AnnotatedHttpServiceElement> 
        find(String pathPrefix, Object object,
	         List<ExceptionHandlerFunction> exceptionHandlersAndConverters,
	         List<RequestConverterFunction> requestConverterFunctions,
	         List<ResponseConverterFunction> responseConverterFunctions) {
  ...
}
anuraaga and others added 27 commits Oct 24, 2019
Motivation:
Related #1645
`useOpenSsl` in `Flags` is used only when TLS is enabled. However, it takes a lot to instantiate `OpenSsl` class so it adds up a tremendous time to the server startup time. We should fix to instantiate it only when it needs.

Modification:
- Set `useOpenSsl` in `Flags` lazily
- Check whether the OS is Linux before calling `Epoll.isAvailable()`
- Add `OsType`

Result:
- Reduced server startup time to 80 percent (20% decreased)
- BOM
  - Use Micrometer BOM 1.3.0
- Brave 5.7.0 -> 5.8.0
- Dropwizard 4.1.0 -> 4.1.1
- gax-grpc 1.49.0 -> 1.49.1
- gRPC 1.24.0 -> 1.24.1
- Netty 4.1.42 -> 4.1.43
- org.bouncycastle 1.63 -> 1.64
- Proguard 6.1.1 -> 6.2.0
- Prometheus 0.7.0 -> 0.8.0
- RxJava2 2.2.12 -> 2.2.13
- Spring Boot 2.1.8 -> 2.1.9
- Tomcat
  - 9.0.26 -> 9.0.27
  - 8.5.43 -> 8.5.47
- ZooKeeper 3.5.5 -> 3.5.6
- Build
  - Gradle 5.6.2 -> 5.6.3
… change. testing is pending. committing before leaving.
…oving

decorating AnnotatedHttpServiceElement to its own class.
@sivaalli

This comment has been minimized.

Copy link
Contributor Author

sivaalli commented Oct 30, 2019

Sorry, I have again messed with the commits. I will create a new PR fixing things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.