Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Added service multiplexing support. #88

Merged
merged 2 commits into from
Feb 10, 2015
Merged

Added service multiplexing support. #88

merged 2 commits into from
Feb 10, 2015

Conversation

tmehlinger
Copy link
Contributor

This is a re-do of #79.

This introduces a TMultiplexingProcessor class which extends TProcessor. This adds the ability for developers to develop services that implement multiple Thrift interfaces. It is inspired by the TMultiplexedProcessor from the Thrift Java library.

Example here: http://stackoverflow.com/questions/19614648/service-multiplexing-using-apache-thrift

@tmehlinger
Copy link
Contributor Author

I checked the build failures and they're in unrelated code. Confirmed tests pass for these changes and that it passes PEP8.

@lxyu
Copy link
Contributor

lxyu commented Feb 9, 2015

@tmehlinger it's related to recent PEP8 changes. The develop branch was upgraded to latest version just now.

Can you please try rebase the branch on develop and fix the pep8 related problem?

# convert kwargs to args
api_args = [args.thrift_spec[k][1]
for k in sorted(args.thrift_spec)]
call = lambda: getattr(self._handler, api)(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a E731 "do not assign a lambda expression, use a def" violation in here.

I've updated the code in develop branch to use the def.

@lxyu
Copy link
Contributor

lxyu commented Feb 9, 2015

👍

LGTM, I'll merge it once the ci passed.

@tmehlinger
Copy link
Contributor Author

Sure thing, I'll have it in shortly.

Travis Mehlinger added 2 commits February 9, 2015 16:39
This introduces a `TMultiplexingProcessor` class which extends `TProcessor`.
This adds the ability for developers to develop services that implement
multiple Thrift interfaces. It is inspired by the `TMultiplexedProcessor`
from the Thrift Java library.
@tmehlinger
Copy link
Contributor Author

Done. It failed to clone the repository on one of the test envs but everything else looks good.

lxyu added a commit that referenced this pull request Feb 10, 2015
Added service multiplexing support.
@lxyu lxyu merged commit 2eb247f into Thriftpy:develop Feb 10, 2015
@lxyu
Copy link
Contributor

lxyu commented Feb 10, 2015

Yah, sometimes travis ci do have some of the envs failed. I triggered re-run and it passed now.

@tmehlinger
Copy link
Contributor Author

👍 thanks!

@xvblack
Copy link
Contributor

xvblack commented Mar 20, 2015

Hi, I am curious that if there is any reason that one service can be only registered once in one server?

I saw that service registration is done by adding functions to a dict, and one service can be only registered once, and requires no Multiplexing in client protocol. This is different from the solution in the original apache issue: https://issues.apache.org/jira/browse/THRIFT-563, which basically allow registering multiple instances of same service using different name.

Is there any plan to support the latter flavor?

@lxyu
Copy link
Contributor

lxyu commented Mar 26, 2015

@xvblack yah you're right, the implementation is different. I'd also prefer the upstream implementation to stay compatible with Apache Thrift.

@tmehlinger how do you think about this idea? The Apache solution is a little more complex but actually avoided the already a registered method naming collision.

@tmehlinger
Copy link
Contributor Author

@lxyu I think it's a great idea. I hadn't actually thought of a good way to resolve the name collision so I took the easy way out. An upstream-compatible solution is going to be way better than what I slapped together.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants