Skip to content

[DNM] sql: grpc prototype#2381

Closed
tamird wants to merge 3 commits intocockroachdb:masterfrom
tamird:sql-grpc
Closed

[DNM] sql: grpc prototype#2381
tamird wants to merge 3 commits intocockroachdb:masterfrom
tamird:sql-grpc

Conversation

@tamird
Copy link
Copy Markdown
Contributor

@tamird tamird commented Sep 5, 2015

So this pretty much works. You can run the server in dev mode and then connect to the sql shell with ./cockroach sql --dev --protocol grpc --addr :8081.

Thoughts?

Comment thread sql/http_server.go Outdated
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.

Can you move these changes into a separate PR? They seem unrelated to the gRPC addition and worth doing independently.

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.

sure.

@petermattis
Copy link
Copy Markdown
Collaborator

Is there any benefit to using gRPC instead of http given that we're not using any of the fancy gRPC features?

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Sep 6, 2015

I'm not totally sure. I think some of the gRPC features are free, like automatic reconnect. Also, many of the fancy features can be used just from the client side, so driver authors may use them even if we do not.

Finally, it should not be taken for granted that driver authors will have to actually implement our protobuf/json-over-http protocol, which may be a non-trivial amount of work in some languages. Presumably, gRPC takes away much of this boilerplate.

@petermattis
Copy link
Copy Markdown
Collaborator

What client-side features of gRPC do you imagine driver authors would use without support from the server? I can see response streaming being useful, but that requires us to support it on the server.

A bigger question in my mind is the performance of gRPC. My recollection was that in Go the performance of gRPC was about the same as HTTP which was significantly worse than Go RPC. I'm pretty sure performance will be a trump card here (trump in the bridge sense, no relation to Donald).

On the other hand, I don't feel we need to be overly restrictive with new transports (I'm sure @bdarnell might chime in now). I do think gRPC support, if added, should be enabled on an experimental basis similar to the old --experimental-rpc flag that controlled whether the KV endpoints could be accessed via Go RPC.

Comment thread server/server.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This loses the host/IP from the input address.

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.

fixed this

@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Sep 7, 2015

The http-specific parts of sql/driver are pretty simple; I'm skeptical that grpc will be substantially easier to implement than protobufs or json over http.

If grpc's performance were comparable to our internal rpc mechanism, then I'd be in favor of ripping out cockroach/rpc and using grpc for everything internally. Last time we looked that wasn't true, but I don't think there are any fundamental obstacles to getting there. Also, if we use (or anticipate using) streaming or other fancier features, we'd do well to use grpc instead of building them into our own custom protocol (and I think there's a good chance we'll want streaming features eventually, although we're a long way from being able to use them effectively today).

My concern with grpc is that it's complex. A lot of the heavy lifting is done at the HTTP/2 layer, but this means that grpc demands a lot of esoteric features from the HTTP/2 layer (like trailers, which I've never seen used in the wild and many HTTP implementations simply pretend don't exist). As long as google is providing the libraries this doesn't matter, but I'm not sure how many high-quality grpc libraries there will be in languages that aren't getting the first-class treatment from google. And the languages where google provides a first-class grpc implementation are also the ones where we will want to provide the best-performing option.

Ultimately, I agree with @petermattis that it comes down to performance. If grpc performs well enough, let's use it everywhere. If not, then I see little value in offering it as an alternative.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Sep 25, 2015

I've updated https://github.com/cockroachdb/rpc-bench since the discussion here took place. Also, let's not forget that Go RPC is not an option for the SQL interface since it needs to work for other languages.

gRPC seems to be currently a bit faster than HTTP, so it may be worth exposing on the SQL interface, even now.

@petermattis
Copy link
Copy Markdown
Collaborator

Go RPC is an option for other languages, it is just more work than using gRPC or HTTP. :)

Interesting that gRPC pulled ahead of HTTP. Sad that it is still so much slower than Go RPC. It would be one thing if it were 10-20% slower. But its 3x slower.

Btw: s/Troughput/Throughput/g.

@bdarnell
Copy link
Copy Markdown
Contributor

Our RPC layer is no longer really Go RPC - the wire protocol is entirely different and there's not even much of the net/rpc package left in our usage. So it's an option to implement this protocol in other languages (albeit not a very attractive one compared to using a protocol that someone else has already implemented).

How hard is it to do the HTTP tests with HTTP/2? Is the http2 package a drop-in replacement for net/http? (don't spend much time on this if it's not)

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Sep 25, 2015

It's hard. https://godoc.org/golang.org/x/net/http2 just implements the protocol, it's not designed to be used directly.

@tamird
Copy link
Copy Markdown
Contributor Author

tamird commented Oct 21, 2015

Closing this to clean up the queue since it doesn't appear we're ready to merge it.

@tamird tamird closed this Oct 21, 2015
@tamird tamird deleted the sql-grpc branch March 8, 2016 22:56
@gattytto
Copy link
Copy Markdown

hello this sounds very interesting for my usecase, all my apps use grpc and python protobuf messages, I also am storing the raw protobuf data in s3 style bucket objects, it would be nice if we could submit protobuf schemas for messages and maybe add the google field_behavior option fields etc.

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.

5 participants