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

proposal: freeze net/rpc #16844

Closed
robpike opened this issue Aug 23, 2016 · 19 comments

Comments

@robpike
Copy link
Contributor

commented Aug 23, 2016

The package has outstanding bugs that are hard to fix, and cannot support TLS without major work. So although it has a nice API and allows one to use native Go types without an IDL, it should probably be retired.

The proposal is to freeze the package, retire the many bugs filed against it, and add documentation indicating that it is frozen and that suggests alternatives such as GRPC.

@robpike robpike added the Proposal label Aug 23, 2016

@mattn

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

retiring jsonrpc too?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

No. JSON-RPC is at least kinda an external spec. Let's not conflate the two here.

@rhedile

This comment has been minimized.

Copy link

commented Aug 23, 2016

Whats the problem with TLS? Been using the Package with TLS and jsonrpc for
18 months for internal distribution without problems, with this recipe
https://gist.github.com/artyom/6897140

Could someone please formalise the term "freeze". the suggested link into
the wiki returns "not found"

On 23 August 2016 at 02:28, Rob Pike notifications@github.com wrote:

The package has outstanding bugs that are hard to fix, and cannot support
TLS without major work. So although it has a nice API and allows one to use
native Go types without an IDL, it should probably be retired.

The proposal is to freeze the package, retire the many bugs filed against
it, and add documentation indicating that it is frozen and that suggests
alternatives such as GRPC.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#16844, or mute the thread
https://github.com/notifications/unsubscribe-auth/AEAf7NjnEUYC1RUdPjP11jWSKgbK1EKsks5qij6ygaJpZM4JqcDQ
.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

I'm in favor of the proposal for the following reason,

The package has outstanding bugs that are hard to fix, and cannot support TLS without major work.

Also because the standard library where its API cannot evolve is not a good place for it.

But I think the value of the following fact is significantly understated.

allows one to use native Go types without an IDL

So I think that grpc is not a viable alternative for anyone who wants to preserve that property. It's a fine suggestion for many.

But, a more net/rpc-like package with support for native Go types would be better developed (by those who are interested) outside of the std lib tree, where it's API could improve to support context.Context, etc.

@peterbourgon

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

I agree with @shurcooL that the value of using Go native types without an IDL is very high, even if just for introductory and prototype purposes. I would hate to see net/rpc removed because it was difficult to satisfy some sophisticated use cases. Maybe it can just be frozen, and appropriate warnings issued in the package documentation?

edit: I misinterpreted retired as removed; mea culpa

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Nobody is proposing violating the Go 1 compatibility promise and removing anything.

@robpike

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

Exactly. As I wrote, "the proposal is to freeze the package".

@adnaan

This comment has been minimized.

Copy link

commented Aug 24, 2016

The following anecdotes of my experience with net/rpc and GRPC only makes sense if my understanding that freezing net/rpc would impact the growth of net/rpc/jsonrpc is correct? If not, I just wasted 15 minutes writing this. Here goes:

I have worked with both net/rpc/jsonrpc and grpc in my projects. Incidentally both use cases were: Go server <-> JSON RPC Java Client. The first use case was to net/rpc/jsonrpc on localhost for automation testing of a phone (uiautomator) and second was a proper Android app using GRPC's streaming capabilities.

In my experience, setting up net/rpc/jsonrpc was trivial, and there was enough Java JSONRPC support to be found while GRPC took a bit of work setting up.

  1. Firstly you need to premeditate your API design in the IDL which does sounds nice but is a pain if you want to get a prototype done quickly.
  2. Even though I was able to setup TLS on Android, till day I am not sure whether the setup was right since the documentation is lacking(checked again, the documentation is in the same state).
  3. I had to write "weird" extra code to utilize the blockingStub in an executor pool.

I guess points 2 & 3 stem from the fact that there hasn't been enough work on GRPC to form a standard body of work. Also I realise 2 & 3 are client issues but when you choose GRPC the client libraries get chosen for you by default.

net/rpc/jsonrpc provides a lot of flexibility of choice while picking a client library and is quicker and easier to use.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

The following anecdotes of my experience with net/rpc and GRPC only makes sense if my understanding that freezing net/rpc would impact the growth of net/rpc/jsonrpc is correct?

False. See the first two replies in this thread.

@sbunce

This comment has been minimized.

Copy link

commented Aug 24, 2016

SGTM on freezing net/rpc.

The net/rpc code is nice and simple. The interfaces are well designed. I use net/rpc with proto codecs to avoid the complexity and dependencies of grpc. Like rhedile mentioned TLS can be used without much difficulty. Freezing doesn't effect these use cases.

@fwessels

This comment has been minimized.

Copy link

commented Sep 6, 2016

We are currently using net/rpc in a.o. https://github.com/minio/minio and https://github.com/minio/dsync and are pretty happy with it, but are open to switching (and getting better performance as a bonus).

Are there any performance comparisons between net/rpc and grpc? Also Golang is (strangely?) missing from the benchmarking results as per eg https://performance-dot-grpc-testing.appspot.com/explore?dashboard=5712453606309888&widget=828849126&container=1808124444&maximized

(BTW there are huge gaps between C++ at 250K QPS/sec vs Java at 150K QPS/sec vs C# at 20K QPS/sec -- especially the last two I would expect to be similar. And these are all async calls, in our case we'd be using synchronous calls).

@quentinmit quentinmit added this to the Proposal milestone Sep 6, 2016

@fwessels

This comment has been minimized.

Copy link

commented Sep 7, 2016

We've done (using https://github.com/minio/dsync) some performance comparisons between net/rpc and grpc and we’ve been surprised to find that grpc is slower by about 4x (given same CPU load).

The following table lists the results of a test with 8 EC2 servers that shows that grpc gives roughly half the results at double the CPU load.

Package EC2 Instance Type Servers Locks/server/sec Total Locks/sec CPU Usage
net/rpc c3.2xlarge 8 (min=1884, max=2096) 15920 25%
grpc c3.2xlarge 8 (min=798, max=923) 6884 55%

To verify that the actual (distributed) locking functionality is not influencing the tests negatively, we repeated the measurements without any server side logic (all code commented out) which gave these results (note that a single lock takes 16 messages, namely 8 locks plus 8 unlocks):

Package EC2 Instance Type Servers Messages/server/sec Total Msgs/sec CPU Usage
net/rpc c3.2xlarge 8 (min=43974, max=53790) 391056 30%
grpc c3.2xlarge 8 (min=20177, max=22272) 169796 60%

Again the grpc performance is about half that of net/rpc at double the CPU load. (Note that the grpc measurements were done using a separate branch, see https://github.com/fwessels/dsync/tree/performance-grpc).

We also found another reference graph that shows "Go sync" performance: https://performance-dot-grpc-testing.appspot.com/explore?dashboard=5760820306771968&widget=828849126&container=1808124444
This shows that the performance for “Go sync” is about 48K QPS for an 8 core server (equivalent of 6K/per core...) which seems to be inline with the results that we are getting. C++ is close to an order of magnitude faster (albeit async) which looks more like it.

So it looks like grpc adds significant overhead compared to net/rpc so we’ll hold off on migrating just yet…

Are there any plans to bring the performance of grpc up to par with net/rpc ?

@pkieltyka

This comment has been minimized.

Copy link

commented Sep 24, 2016

@fwessels it would be interesting to see the performance difference with grpc using the serialization code generated by https://github.com/gogo/protobuf -- I think it'll show that grpc has lots of potential to get optimized

@fwessels

This comment has been minimized.

Copy link

commented Sep 26, 2016

@pkieltyka I would doubt whether this would make a big difference as the arguments in the test are really simple (just a string in and bool as return value). Do you have an idea how much difference it could make?

@robmccoll

This comment has been minimized.

Copy link

commented Oct 5, 2016

cannot support TLS without major work

Perhaps I'm missing something, but is it not as simple as using either Accept or ServeConn with a TLS net.Listener or TLS net.Conn?

listener, _ := tls.Listen("tcp", ":1234", config)
rpc.Accept(listener)

As you said, nice API, no need for an IDL or other dependencies, and I would add simple, composable, and powerful. It's among the easiest RPC mechanisms to implement for Go, scales well, and achieves high throughput with low latency. grpc to me isn't really an alternative as it's design is focused on completely different goals. grpc provides many additional capabilities, but at the cost of the things that make net/rpc desirable.

@danmux

This comment has been minimized.

Copy link

commented Oct 5, 2016

I am confused by the difference between 'deprecate' and 'freeze', is there any formality about these terms. I would typically see deprecation as frozen hoping for the ability to remove at some future time.

The complaint about https is only partly valid, I agree it is annoying to find the recipe but once found does not change much - this could be added to the examples.

I agree with others that that net/rpc is lightweight fast simple and serves many scenarios well.

I have not suffered from any of what @robpike refers to as "outstanding bugs that are hard to fix," so would be happy for these to be migrated to documented known limitations, and avoid going to any great lengths to fix these bugs.

for the reasons @robmccoll mentions I think it would be wrong to encourage folk to use alternatives like GRPC in place of net/rpc and the word 'deprecate' would discourage the otherwise perfectly sensible use of this simple useful package.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

I think everybody only means "freeze".

@robpike robpike changed the title proposal: deprecate net/rpc proposal: freeze net/rpc Oct 5, 2016

@adg adg modified the milestones: Go1.8, Proposal Oct 24, 2016

@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

Freeze it!

@gopherbot

This comment has been minimized.

Copy link

commented Oct 26, 2016

CL https://golang.org/cl/32112 mentions this issue.

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