Skip to content

RFC: Switch to GRPC.#3352

Merged
bdarnell merged 1 commit intocockroachdb:masterfrom
bdarnell:grpc-rfc
Jan 4, 2016
Merged

RFC: Switch to GRPC.#3352
bdarnell merged 1 commit intocockroachdb:masterfrom
bdarnell:grpc-rfc

Conversation

@bdarnell
Copy link
Copy Markdown
Contributor

@bdarnell bdarnell commented Dec 8, 2015

Review on Reviewable

@bdarnell
Copy link
Copy Markdown
Contributor Author

bdarnell commented Dec 8, 2015

CC @YuleiXiao

Comment thread docs/RFCS/grpc.md
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/be/will be/

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Dec 8, 2015

Seems like we get the important benefits at a substantially lower development cost by going to protobuf over HTTP2. We can then switch to GRPC when it becomes compelling in its own right (streaming, perf, adoption).

@bdarnell
Copy link
Copy Markdown
Contributor Author

bdarnell commented Dec 8, 2015

The performance cost is there whether it's GRPC or plain HTTP/2. And it looks like streaming would have better performance than regular RPCs (no headers), so the performance penalty of GRPC would be smaller than rpc-bench suggests once we move our raft transport to streaming.

I think that the streaming interface is sufficient reason to choose GRPC over HTTP/2 for internal-only use. The main reason I would choose plain HTTP/2 would be for public APIs, since if GRPC fails to see wide adoption there may not be enough client libraries for it.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Dec 9, 2015

Another issue with GRPC is grpc/grpc-go#317

There's no way to track connections as with net/http's ConnState, so shutting things down cleanly in tests will be troublesome.

@bdarnell
Copy link
Copy Markdown
Contributor Author

Some very rough performance estimates: The main contributor to the performance difference is that GRPC performs 50% more Write syscalls (GRPC does two writes on the server side and one on the client; our own codec does one in each direction). Therefore, we can estimate the performance impact of a move to GRPC by multiplying the time spent in write syscalls by 1.5.

The benchmarks in the sql package spend between 15 and 30% of their time in write syscalls for RPCs (on a mac; I've hit some obstacles in trying to get a benchmark on linux for comparison). So if we cannot eliminate the redundant syscall in grpc, the overall performance impact would be around 8-15%.

@petermattis
Copy link
Copy Markdown
Collaborator

The GRPC performance can be fixed. Whether those fixes will be accepted upstream is a different question.

Comment thread docs/RFCS/grpc.md
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FYI, streaming gRPC calls are faster than non-streaming calls. I added something to Tamir's benchmark at some point, but never merged the results. I don't remember the details only that the difference was significant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's because streaming RPCs don't include headers with every message, so it's just one Write syscall per message. I haven't benchmarked it but on paper streaming mode should be pretty close to our current system. I don't think we can make everything use the streaming mode but it's a good fit for at least raft and gossip.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll find and resurrect my change to rpc-bench so that we have real numbers to look at.

bdarnell added a commit that referenced this pull request Jan 4, 2016
@bdarnell bdarnell merged commit a4343f0 into cockroachdb:master Jan 4, 2016
@bdarnell bdarnell deleted the grpc-rfc branch January 4, 2016 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants