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

vendor: update grpc-go #14939

Merged
merged 2 commits into from
Apr 17, 2017
Merged

vendor: update grpc-go #14939

merged 2 commits into from
Apr 17, 2017

Conversation

asubiotto
Copy link
Contributor

The motivation behind this update is to include andreimatei/grpc-go@ded79a3
which avoids having one RPC hog the connection and allows for a
decent number of RPCs on the same connection. Some DistSQL queries
were hanging because of some RPCs being blocked on the consumption of
other RPCs which couldn't use the connection to progress.

The motivation behind this update is to include andreimatei/grpc-go@ded79a3
which avoids having one RPC hog the connection and allows for a
decent number of RPCs on the same connection. Some DistSQL queries
were hanging because of some RPCs being blocked on the consumption of
other RPCs which couldn't use the connection to progress.
@asubiotto asubiotto requested review from dt and tamird April 14, 2017 18:57
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator

Some DistSQL queries were hanging because of some RPCs being blocked on the consumption of other RPCs which couldn't use the connection to progress.

Is there an issue which describe the specific deadlock scenario that is being encountered? I'm interested in understanding more precisely what is going on here. It seems like the deadlock can still occur and could have occurred even with the original gRPC window settings. That makes this change a bit of a kludge (which I'm perfectly ok with in the short term).

@dt dt removed their request for review April 14, 2017 19:11
@asubiotto
Copy link
Contributor Author

Here's the issue #14948. It does let me keep on running TPC-H queries and having the connection window size be a multiple of the RPC window size is probably a good idea in general.

@tamird
Copy link
Contributor

tamird commented Apr 14, 2017

Looks like you'll have to fix a typo while you're here: /go/src/github.com/cockroachdb/cockroach/pkg/cli/context.go:70:36: "itnerface" is a misspelling of "interface"

@asubiotto
Copy link
Contributor Author

Will submit a separate PR for that. I'm kind of confused though, it seems like the typo has been around for a few months?

@dt
Copy link
Member

dt commented Apr 14, 2017

Your update picked up an unrelated update to the spellcheck linter and it started noticing it.

@tamird
Copy link
Contributor

tamird commented Apr 14, 2017 via email

@asubiotto asubiotto mentioned this pull request Apr 14, 2017
@bdarnell
Copy link
Contributor

:lgtm:


Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Copy link
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

I'm looking forward to testing this out!

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.

7 participants