Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

finagle-protobuf #91

Closed
wants to merge 56 commits into
from

Conversation

We made a couple of improvements based on our experience using the protobuf based RPC in production.

George Vacariuc and others added some commits Apr 11, 2012

George Vacariuc added load test, refactored a844e92
George Vacariuc moved method lookup tests to scala 4a7e0eb
George Vacariuc Merge branch 'master' of git://github.com/twitter/finagle d8d8ba0
George Vacariuc added finagle-protobuf pom 5502ea4
George Vacariuc added release() for the stub 397a250
George Vacariuc sbt eclipse support e619a53
George Vacariuc eclipsify finagle-serversets 7e4780d
George Vacariuc temporary workaround for ChannelClosedExceptions raise when servers s…
…hutdown
443040f
George Vacariuc move snapshot b218582
Michael Berman add scm pointer to finagle-protobuf pom be6b50d
Michael Berman [maven-release-plugin] prepare release finagle-protobuf-3.0.2 afda314
Michael Berman [maven-release-plugin] prepare for next development iteration 562b63b
George Vacariuc release 3.0.2 46ef82f
George Vacariuc merged 48a9b19
George Vacariuc preconditions check 4f7e2eb
George Vacariuc handle runtime exceptions and return empty messages 563b64e
Michael Berman [maven-release-plugin] prepare release finagle-protobuf-3.0.4-TENDRIL ff85203
Michael Berman [maven-release-plugin] prepare for next development iteration 85a3b4e
George Vacariuc using a promise for async computations 47d1437
George Vacariuc Merge branch 'master' of https://fisheye.tendrilinc.com/git/finagle 572cbcc
George Vacariuc fixed def to val 997b0a4
George Vacariuc release 3.0.5-TENDRIL 69e9136
Michael Berman add .gitignore 9b40464
@mkochco mkochco bump finagle-protobuf to next snapshot version 40dfc44
George Vacariuc added loggin 0963750
George Vacariuc added loggin 8ccd3d4
George Vacariuc 3.0.7 snapshot 02477fb
George Vacariuc 3.0.7 07a3375
George Vacariuc 3.0.8 SNAPSHOT 0498fd5
George Vacariuc use toString() 9da0f7d
George Vacariuc corrected version 17482dd
George Vacariuc typo fix a333a23
George Vacariuc 3.0.9 added service exception handler 43182e7
George Vacariuc client side exception deserializer 2413d6a
George Vacariuc client side exception deserializer adebac7
George Vacariuc snapshot 74504e8
George Vacariuc scala to java array fix 1085cd1
@cgleissner cgleissner Added Git ignores 21f10fd
George Vacariuc refactored 9c47fe6
George Vacariuc Merge branch 'master' of https://fisheye.tendrilinc.com/git/finagle 908cc00
Contributor

mariusae commented Jul 6, 2012

hey George—I'm currently on vacation and will have a look when I get back (in ~1 week)

George Vacariuc and others added some commits Jul 17, 2012

Hey Marius,

I noticed one of your comments in the finagle-protobuf module
"finagle-protobuf:
kill POM; seems
defunct.a3dd155".
We have been using this protocol in production for months, and it has been
working great for us and we did not need to make any updates.

The one thing I will need to add is a better wiki page.

George

On Fri, Jul 6, 2012 at 1:47 AM, marius a. eriksen <
reply@reply.github.com

wrote:

hey George—I'm currently on vacation and will have a look when I get back
(in ~1 week)


Reply to this email directly or view it on GitHub:
#91 (comment)

Contributor

mariusae commented Nov 1, 2012

I think I was referring to the POM :-)

My chief objection to making finagle-protobuf truly first class at this point is that I'd really like to separate the encoding from the protocol. There are some upcoming changes in finagle-thrift which will make it a very appealing (and generic) RPC transport, upon which we can use protocol buffers, thrift, whatever.

I'll probably factor that out into something called "finagle-rpc"

George Vacariuc added some commits Dec 11, 2012

George Vacariuc fixed tests 2ff98a6
George Vacariuc typo 9c9755c
George Vacariuc code format a0b37f7

mkochco commented Jul 15, 2013

Hi Marius,

What's the status of your efforts around generalizing finagle-thrift ( i.e. finagle-rpc )?

Thanks,
Mark

@mkochco mkochco Merge remote-tracking branch 'github/master'
Conflicts:
	.gitignore
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/RpcControllerWithOnFailureCallback.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/RpcFactory.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/RpcServer.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/Util.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/channel/ProtobufEncoder.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/impl/RpcChannelImpl.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/impl/RpcFactoryImpl.scala
	finagle-protobuf/src/main/scala/com/twitter/finagle/protobuf/rpc/impl/RpcServerImpl.scala
	finagle-protobuf/src/test/scala/com/twitter/finagle/protobuf/rpc/LoadSpec.scala
	finagle-protobuf/src/test/scala/com/twitter/finagle/protobuf/rpc/SampleServiceProtos.java
	finagle-protobuf/src/test/scala/com/twitter/finagle/protobuf/rpc/SimpleMethodLookupSpec.scala
	project/build/Project.scala
	project/plugins/Plugins.scala
f2c291c
Contributor

mariusae commented Jul 15, 2013

This is done: finagle-mux. As an example of adapting Mux to Thrift, see finagle-thriftmux

Contributor

mariusae commented Aug 5, 2013

@george-vacariuc how do you feel about closing this, and porting just the serialization code to use Mux instead? I promise, it is much better.

@mariusaeriksen we definitely want to keep in step with the framework, let me see how I can slip this into our backlog. Thanks for the heads up.

george vacariuc and others added some commits Aug 16, 2013

george vacariuc split protobuf class to make scala happy; added test for service to s…
…ervice interaction
154533d
@george-vacariuc george-vacariuc added server spec to verify on which thread pool SPF side requests ar…
…e dispatched
488e7e0
@george-vacariuc george-vacariuc added comments d7a3fa5
George Vacariuc removed unused spec 80fd22c
George Vacariuc added logging statements back 64f5cb3
George Vacariuc callbacks can be dispatched in a separate thread 4297889
George Vacariuc updated version 6.5.3-TENDRIL 76fa8d6
Contributor

mosesn commented Dec 20, 2013

@george-vacariuc any update on porting this to mux?

@mosesn unfortunately, at this time, I do not have the bandwidth to do the porting.

Contributor

mosesn commented Dec 22, 2013

ok, keep us posted. thanks!

Hi @mosesn,

We are interested in moving forward with finagle-protobuf and we have some bandwidth to make it happen. One of our biggest motivators is a desire to utilize zipkin for distributed tracing of our finagle protobuf services. I've taken a look at the mux transport and it appears to provide the tracing support we desire. Is there a simple example, or maybe some tests, that show a simple implementation on top of the mux transport? Any help would be much appreciated.

Thanks,
Patrick

Contributor

bmdhacks commented Mar 19, 2014

Not so much. Mux is still pretty new in Finagle. The only current
implementation is thriftmux. I agree though that protobuf+mux would be an
extremely useful protocol.

On Wed, Mar 19, 2014 at 8:40 AM, Patrick Osborne
notifications@github.comwrote:

Hi @mosesn https://github.com/mosesn,

We are interested in moving forward with finagle-protobuf and we have some
bandwidth to make it happen. One of our biggest motivators is a desire to
utilize zipkin for distributed tracing of our finagle protobuf services.
I've taken a look at the mux transport and it appears to provide the
tracing support we desire. Is there a simple example, or maybe some tests,
that show a simple implementation on top of the mux transport? Any help
would be much appreciated.

Thanks,
Patrick

Reply to this email directly or view it on GitHubhttps://github.com/twitter/finagle/pull/91#issuecomment-38065964
.

Contributor

mariusae commented Mar 19, 2014

finagle-thriftmux is a simple implementation showing how to compose an RPC system on top of Mux.

Contributor

bmdhacks commented Mar 19, 2014

That being said, you might look at the api docs:
http://twitter.github.io/finagle/docs/#com.twitter.finagle.mux.package

On Wed, Mar 19, 2014 at 10:14 AM, Brian Degenhardt bmd@twitter.com wrote:

Not so much. Mux is still pretty new in Finagle. The only current
implementation is thriftmux. I agree though that protobuf+mux would be an
extremely useful protocol.

On Wed, Mar 19, 2014 at 8:40 AM, Patrick Osborne <notifications@github.com

wrote:

Hi @mosesn https://github.com/mosesn,

We are interested in moving forward with finagle-protobuf and we have
some bandwidth to make it happen. One of our biggest motivators is a desire
to utilize zipkin for distributed tracing of our finagle protobuf services.
I've taken a look at the mux transport and it appears to provide the
tracing support we desire. Is there a simple example, or maybe some tests,
that show a simple implementation on top of the mux transport? Any help
would be much appreciated.

Thanks,
Patrick

Reply to this email directly or view it on GitHubhttps://github.com/twitter/finagle/pull/91#issuecomment-38065964
.

Thanks for the responses. The mux protocol documentation has been helpful.

I've been making a bit of progress sorting out the thriftmux implementation and what a protobuf implementation may look like.

I have a question about ClientBuilder and ServerBuilder, are those classes intended only for non-mux implementations? or am I missing something there. Is there a similar mux mechanism to config the clients and servers?

Thanks,
Patrick

Member

roanta commented Mar 20, 2014

The builders aren't compatible with mux, but we're working on making them more general so that should change in the near future. However, a lot of the current builder knobs become meaningless with protocols like mux [1]. Because of this, the new apis were designed to a be a bit more rigid and protocol implementors are expected to provide sensible defaults (ex. [2]).

[1] http://twitter.github.io/finagle/guide/FAQ.html#mux-specific-faq
[2] http://twitter.github.io/finagle/guide/FAQ.html#how-do-i-change-my-timeouts-in-the-finagle-6-apis

Thanks, the FAQs are very helpful.

What about adding a tracer to client and/or server? We are in need of distributed tracing and our hoping to use Zipkin in that effort with the new protobuf built on mux.

-- Patrick

So what's the best option for protobufs w/finagle at the moment?

Thanks,
Justin

Our company has tried to push our version back to Finagle but we got some pushback because it was not built on the mux transport (which wasn't complete at the time). So other than building the functionality yourself, you may need to wait till we get our new version complete and pushed back to the open source community.

If you have some experience with protoc plugins for generating Java code that would be very useful to producing a canonical Finagle / Protobuf interface. See my post on the protobuf google group here:

https://groups.google.com/forum/#!topic/protobuf/fnKqCcT9NXo

Cheers,
Patrick

Hi All,

I have built a basic protobuf implementation on the mux transport and it turned out to be fairly easy using thriftmux for some inspiration.

I'm now working on getting the tracing to work with the new implementation. I have added Tracers to the DefaultClient and DefaultServer created with the mux transporter but the service name and method name aren't filled in. So it looks like I need to call something like:

  Trace.recordRpcname(serviceName, methodName)

after the TracingFilter executes and pushes the Tracer and sets up the trace id. But I'm not sure how best to insert this call into client and server instances.

I've read through much of the thriftmux implementation and I have been unable to find how it is done there. Is thriftmux recording the service name and method name somehow that I am missing?

It does look like in the DefaultServer the 'prepare' field (ServiceFactory) could be used to record the rpc information. Is that correct? Or is there a better place to do that?

On the client side, I haven't found some sort of hook to use yet. Is there something in DefaultClient I can use? or maybe the ClientDispatcher?

Thanks for the help,
Patrick.

Contributor

bmdhacks commented Mar 27, 2014

I'm willing to bet that recordRpcname is not called in mux and that's a bug. Lemme look into it some more.

Thanks for the quick response.

My thinking is that the mux transport has no way of knowing what the service name and method name should be. So that is why I was looking for some sort of hook in the client and server to provide the information.

Also on a side note, I've been looking at thriftmux and the fact that the client and server builders aren't compatible with mux and I'm starting to wonder if anyone has used the mux transport in production.

Is mux just experimental at this point or is it production ready?

Thanks again,
Patrick

Contributor

mariusae commented Mar 27, 2014

Also on a side note, I've been looking at thriftmux and the fact that the client and server builders aren't compatible with mux and I'm starting to wonder if anyone has used the mux transport in production.

The intention is to use the new-style APIs; ClientBuilders and ServerBuilders are not going to be provided for new protocols. @roanta has a change coming—it should hit github very soon—which allows the same kind of customization with the new APIs.

@bmdhacks bmdhacks closed this Mar 27, 2014

@bmdhacks bmdhacks reopened this Mar 27, 2014

Thanks Marius. I understand not using the builders with the new protocols. In the meantime I've passed the tracer into the default client and server.

The driver behind us moving the protobuf implementation to mux is so that we can hook up Zipkin for distributed tracing in production. So I have two primary concerns:

  • getting the tracing working with the service and method names
  • and knowing that the mux transport is production ready

Do you have some thoughts on that?

Thanks,
Patrick

Contributor

bmdhacks commented Mar 28, 2014

Both of these issues are huge focusses for the finagle team at twitter, and we're actively responding to any issues that arise with them. I'm not promising it's all bug-free, but we're extra-responsive on this stuff this quarter. You're on the cutting edge, but I don't think you're too experimental to be in production.

Great. Thanks for the info. If I can help with the mux code let me know.

I'm working on integrating the protobuf mux implementation with our existing services to see how the implementation shakes out. I'll let you know if I see any other issues.

Hi @bmdhacks,

Do you have an update on the rpc service name and method name issue with the mux transport?

Should I open an issue?

Thanks,
Patrick

Is there an update on the mux tracing issue? Should I open an issue?

Thanks,
Patrick

Contributor

bmdhacks commented Apr 1, 2014

It's fixed internally, we're just working on getting a release out with the code.

what is the timeframe for the tracing fix?

-- Patrick

Contributor

bmdhacks commented Apr 9, 2014

We'll have something up within a week

@bmdhacks bmdhacks closed this Apr 9, 2014

@bmdhacks bmdhacks reopened this Apr 9, 2014

Member

stuhood commented Apr 9, 2014

@PatrickOsborne: Would you mind updating this pull request, or opening a new one with your implementation? We might as well begin the review process while we wait for that fix. Also, I don't think the fix actually blocks landing the patch?

Contributor

bmdhacks commented Apr 10, 2014

Ok, the mux tracing fix is now pushed

Contributor

mosesn commented Apr 18, 2014

And now it has been merged back into github.

Contributor

mosesn commented Aug 13, 2014

@george-vacariuc we've been thinking about how it's a huge pain that finagle keeps changing underneath your feet so it's hard to merge in finagle-protobuf. We've also been putting similar projects under the finagle [organization][0]. What do you think about the idea of making finagle-protobuf its own project under the finagle org, so that it doesn't have to go through the same stringent code review process? We can also make you an owner of the project, so you'll be able to merge pull requests etc yourself, which makes the most sense since you're the expert on finagle-protobuf.

We'll still be happy to consult on finagle-protobuf, but we won't have to end up in these sinkholes where nothing gets done.

Thoughts?

Moses, that's a good idea. I will check with our compliance people if
they're ok with me working on this and I will follow up with you in the
next week or so.

On Wed, Aug 13, 2014 at 12:00 PM, Moses Nakamura notifications@github.com
wrote:

@george-vacariuc https://github.com/george-vacariuc we've been thinking
about how it's a huge pain that finagle keeps changing underneath your feet
so it's hard to merge in finagle-protobuf. We've also been putting similar
projects under the finagle [organization][0]. What do you think about the
idea of making finagle-protobuf its own project under the finagle org, so
that it doesn't have to go through the same stringent code review process?
We can also make you an owner of the project, so you'll be able to merge
pull requests etc yourself, which makes the most sense since you're the
expert on finagle-protobuf.

We'll still be happy to consult on finagle-protobuf, but we won't have to
end up in these sinkholes where nothing gets done.

Thoughts?


Reply to this email directly or view it on GitHub
#91 (comment).

Contributor

mosesn commented Dec 29, 2014

@george-vacariuc any update from your compliance folks?

Hey Moses,

Please accept my apologies for having not followed up; unfortunately I will
not be able to contribute to the project.

--George

On Mon, Dec 29, 2014 at 10:48 AM, Moses Nakamura notifications@github.com
wrote:

@george-vacariuc https://github.com/george-vacariuc any update from
your compliance folks?


Reply to this email directly or view it on GitHub
#91 (comment).

Contributor

travisbrown commented Mar 13, 2015

For the record, after a conversation with @chrisphelps yesterday I rebased this PR against develop and split finagle-protobuf out into its own project in the Finagle org. It's likely that we'll remove the subproject from this repository, and once that happens I'll close this PR.

Contributor

chrisphelps commented Mar 13, 2015

Thanks, @travisbrown

@travisbrown travisbrown added a commit that referenced this pull request Mar 23, 2015

@travisbrown travisbrown + jenkins Remove unused finagle-protobuf project
Problem

finagle-protobuf is not being compiled, tested, or used at Twitter and is badly
out of date.

Solution

I've created a new finagle-protobuf project in the Finagle organization on
GitHub from the subproject here (after rebasing #91). This commit completes the
move by deleting the unused subproject and removing the commented-out build
config.

Result

finagle-protobuf lives in the Finagle organization.

RB_ID=611556
6a73425
Contributor

travisbrown commented Mar 24, 2015

finagle-protobuf is now a stand-alone project in the Finagle organization and has been removed as a subproject here.

@jbripley jbripley pushed a commit to jbripley/finagle that referenced this pull request Oct 28, 2015

@travisbrown travisbrown + Joakim Bodin Remove unused finagle-protobuf project
Problem

finagle-protobuf is not being compiled, tested, or used at Twitter and is badly
out of date.

Solution

I've created a new finagle-protobuf project in the Finagle organization on
GitHub from the subproject here (after rebasing #91). This commit completes the
move by deleting the unused subproject and removing the commented-out build
config.

Result

finagle-protobuf lives in the Finagle organization.

RB_ID=611556
dd6d2ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment