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

Allow binding more than one Thrift service at the same path #2164

Closed
trustin opened this issue Oct 7, 2019 · 11 comments · Fixed by #2285
Closed

Allow binding more than one Thrift service at the same path #2164

trustin opened this issue Oct 7, 2019 · 11 comments · Fixed by #2285
Milestone

Comments

@trustin
Copy link
Member

trustin commented Oct 7, 2019

A user wants to write a Thrift proxy server that forwards the requests to their corresponding remote Thrift services where each Thrfit service has different IDLs, e.g.

service FooService {
    string foo()
}
service BarService {
    string bar()
}

If a client calls foo() then FooService would be invoked. bar() then BarService.

Currently, a user has to use TMultiplexed protocol and specify a service name explicitly.

Once this feature is implemented, a proxy Thrift server could provide a union/merged view of multiple Thrift services without using TMultiplexed protocol, allowing a user to split/merge their services without affecting the public API.

@trustin
Copy link
Member Author

trustin commented Oct 7, 2019

For example, we could allow a user to build a THttpService from multiple Thrift AsyncIface/Iface implementations rather than just one. We could also accept an optional service name when adding an AsyncIface/Iface implementation to support TMultiplexed protocol as well.

@trustin
Copy link
Member Author

trustin commented Oct 7, 2019

Another food for thought: a user wants to specify different decorators for different Thrift service types. It is currently doable by writing a decorating RpcService which wraps other decorators, e.g.

public class MyRpcDecorator extends SimpleDecoratingRpcService {
    final Service<RpcRequest, RpcResponse> decoratorA;
    final Service<RpcRequest, RpcResponse> decoratorB;
    public MyRpcDecorator(Service<RpcRequest, RpcResponse> delegate) {
        super(delegate)
        decoratorA = new DecoratorForServiceA(delegate);
        decoratorB = new DecoratorForServiceB(delegate);
    }
    @Override
    public RpcResponse serve(ServiceRequestContext ctx, RpcRequest req) throws Exception {
        if (req.serviceType() == ServiceA.class) {
            return decoratorA.serve(ctx, req);
        } else {
            return decoratorB.serve(ctx, req);
        }
    }
}

Maybe we need to find a way to make this kind of diversion easier..

@sivaalli
Copy link
Contributor

I can take a look at this. Is it fine?

@trustin
Copy link
Member Author

trustin commented Oct 30, 2019

@sivaalli Absolutely! Thanks a lot for looking into this. ❤️

@sivaalli
Copy link
Contributor

Hi @trustin , I was able to look into this recently. I had a couple of questions and wanted to reach out to you before starting to write any code.

  1. The idea here is to map a function name to a ThriftFunction right. For example: if we have 2 services such as (taken from above):
service FooService {
    string foo()
}
service BarService {
    string bar()
}

And when a server builder is created like this:

        final Server server = Server.builder()
                                    .port(new ServerPort(8080, SessionProtocol.HTTP))
                                    .service("/", THttpService.of(new FooService(), new BarService()))
                                    .build();

Any request with path / that has TMessage.name matching foo, then FooService.foo should be invoked and matching bar, then BarService.bar should be invoked right. Is my understanding right here?

@trustin
Copy link
Member Author

trustin commented Nov 19, 2019

@sivaalli Thanks for looking into this issue first of all.

Is my understanding right here?

Yes, we should look for a function in the order as specified when building a THttpService.

Also, if FooService and BarService both have foo(), the FooService, which was specified first, will be chosen.

Please also keep in mind that we also need to support TMultiplexed, which is supported via of(Map) factory method. Let me know if you are not familiar with TMultiplexed - I can elaborate on it.

@sivaalli
Copy link
Contributor

sivaalli commented Nov 20, 2019

Thank you for quick response @trustin. I'm currently understanding/learning about how thrift works internally. I was able to understand that TMultiplexed is a way to bind multiple thrift services to the same http path by assigning a service name.

And the service name is passed in client request to target a single thrift service. Is my understanding correct? Please correct me if it is not so.

@trustin
Copy link
Member Author

trustin commented Nov 20, 2019

@sivaalli That's correct! 👍 Ideally, it will be awesome if we have a builder API which allows mapping multiple services with and/or without multiplexing:

THttpService.builder()
             .addService(new FooService())
             .addService(new BarService())
             .addService("a", new AService())
             .addService("a", new BService())
             .build()

If service name does not exist in the request, FooService or BarService will be chosen. If service name is "a", AService or BService would be chosen.

@sivaalli
Copy link
Contributor

This Builder API idea looks good. I will start working on this 😄

@sivaalli
Copy link
Contributor

Hi @trustin, do you think that entire THttpService should support a default serialization format or each service(FooService and BarService) could specify a different default serialization format?

If each service can have a different default serialization format then we could potentially match the function that supports a specific serialization format from the request. Do you think this is necessary?

@trustin
Copy link
Member Author

trustin commented Nov 22, 2019

@sivaalli That's a good question. I think we can add such an option later if requested by users. Meanwhile, let's stick to the current behavior - using the same default/available serialization formats.

trustin pushed a commit that referenced this issue Dec 6, 2019
This PR will allow multiple Thrift services to be bind on an HTTP path, also supporting `TMultiplexed ` protocol.

**Changes**:
1. `ThriftFunction` holds reference of the implementation
2. `ThriftServiceMetadata` now can take a list of thrift service implementations and can hold a mapping between the method name and `ThriftFunction`.
3. Introduced `THttpServiceBuilder` to fluently build instance of `THttpService`.

**Deprecations**:
 1. API's in`THttpService` that take `Map<String, ?>` are deprecated in favor of `THttpServiceBuiler.ofService(String serviceName, Object implementation)`.

**Breaking changes**:
1. `ThriftCallService` factory method with signature `ThriftCallService of(Map<String, ? extends Iterable<?>> implementation)` is changed to `ThriftCallService of(Map<String, ? extends Iterable<?>> implementations)`

Closes: #2164
@trustin trustin added this to the 0.97.0 milestone Dec 8, 2019
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
This PR will allow multiple Thrift services to be bind on an HTTP path, also supporting `TMultiplexed ` protocol.

**Changes**:
1. `ThriftFunction` holds reference of the implementation
2. `ThriftServiceMetadata` now can take a list of thrift service implementations and can hold a mapping between the method name and `ThriftFunction`.
3. Introduced `THttpServiceBuilder` to fluently build instance of `THttpService`.

**Deprecations**:
 1. API's in`THttpService` that take `Map<String, ?>` are deprecated in favor of `THttpServiceBuiler.ofService(String serviceName, Object implementation)`.

**Breaking changes**:
1. `ThriftCallService` factory method with signature `ThriftCallService of(Map<String, ? extends Iterable<?>> implementation)` is changed to `ThriftCallService of(Map<String, ? extends Iterable<?>> implementations)`

Closes: line#2164
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.

2 participants