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

go.mod: bump gRPC and google SDKs #90855

Closed
wants to merge 2 commits into from
Closed

Conversation

rhu713
Copy link
Contributor

@rhu713 rhu713 commented Oct 28, 2022

The first commit bumps gRPC to v1.49.0 as it is needed by google storage v1.27.0.

The second commit bumps cloud.google.com/go/storage to 1.27.0. This version contains the fix for the first request GCS chunked uploads not being retried. Bump pubsub version to maintain compatibility with transitive dependencies.

Release note: None

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Oct 28, 2022

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rhu713 rhu713 force-pushed the gcs-1.27.0 branch 2 times, most recently from 24f5d45 to 16507a9 Compare October 28, 2022 18:53
@rhu713 rhu713 marked this pull request as ready for review October 28, 2022 18:56
@rhu713 rhu713 requested review from a team as code owners October 28, 2022 18:56
@rhu713 rhu713 requested review from benbardin and dt and removed request for a team and benbardin October 28, 2022 18:56
@rhu713 rhu713 requested a review from knz October 31, 2022 14:52
@knz
Copy link
Contributor

knz commented Oct 31, 2022

  1. How did you run this upgrade?

    If you used go get -u you've taken on too many upgrades.

    The proper way would be e.g. go get cloud.google.com/go/storage@1.27.0

    (If you've seen -u mentioned in instructions, please tell us. We might need to update the instructions. That -u is generally undesirable - see build/README: tell folk to avoid -u #90984)

  2. If the updates to grpc and protobuf are absolutely required, then let's pull those off in a separate PR that would run first. We want to see a clear CI + run a more careful examination of changes there, separately from your change.

@dt
Copy link
Member

dt commented Oct 31, 2022

@knz: Google cloud storage 1.27 explicitly depends on grpc v1.49.0, but there is a breaking api change in that they unbroke in 1.50, which is why we're going to that instead of 1.49. different library, nvm

All the changed deps here are either first-party golang libraries or google's. The only one that really catches my attention is grpc, but as mentioned above, that one is forced, so not much to do there other than try it and let the roachtests see what they think.

@knz
Copy link
Contributor

knz commented Oct 31, 2022

not much to do there other than try it and let the roachtests see what they think.

See my previous message:

let's pull those off in a separate PR that would run first. We want to see a clear CI + run a more careful examination of changes there, separately from your change.

@knz
Copy link
Contributor

knz commented Oct 31, 2022

Given the amount of infra dep changes here, we certainly need to look at this more carefully. It's a shame that the github review interface makes this difficult.

@dt
Copy link
Member

dt commented Oct 31, 2022

What would be the benefit of splitting this into separate PRs? It is already only a dep bump, not feature code changes.

If we land it as is an roachtests suggest there is an issue, then we revert it but know which test to use when bisecting library-by-library/version-by-version. Doing a piecemeal bump when we don't even know if or what we'd be worried about doesn't seem required (nor is it what we've done in the past)

@knz
Copy link
Contributor

knz commented Oct 31, 2022

What i'd like us to achieve is a standard by which grpc/protobuf/etc upgrades "stick out" in some fashion, and get extra attention by virtue of sticking out on their own.

Currently we are unable to get that extra attention because they get bundled with other things that seem innocuous, like this PR.

How do you suggest getting that extra attention, short of a separate PR with a distinctive title? (Like "upgrade gRPC to version X, protobuf to version Y")?

@dt
Copy link
Member

dt commented Oct 31, 2022

How about:
a) do it in two commits (since log is what most people will look at after the fact) so gRPC gets its own
b) retitle the PR to "go.mod: bump gRPC and google sdk" or similar to stick out more?

@knz
Copy link
Contributor

knz commented Oct 31, 2022

yes that would be an improvement

Rui Hu added 2 commits November 2, 2022 18:29
Release note: None
Bump cloud.google.com/go/storage to 1.27.0. This version contains the fix for
the first request GCS chunked uploads not being retried. Bump pubsub version
to maintain compatibility with transitive dependencies.

Release note: None
@rhu713 rhu713 changed the title vendor: cloud.google.com/go/storage to 1.27.0, pubsub to v1.25.1 go.mod: bump gRPC and google SDKs Nov 3, 2022
@rhu713
Copy link
Contributor Author

rhu713 commented Nov 3, 2022

hey @knz / @dt. I separated the gRPC v1.49.0 bump into its own commit and updated the PR title and commit descriptions. PTAL when you get a chance. Thanks!

@knz
Copy link
Contributor

knz commented Nov 6, 2022

I've looked at the gRPC bump and checked for the following:

  • no licensing changes
  • no major configurability changes (crdb tweaks quite a few options - none of them are affected here)

However there's quite a few changes related to dial and retries. i've asked a double check from the KV team here: https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1667737349258559

@knz
Copy link
Contributor

knz commented Nov 6, 2022

Quoth @erikgrinaker :

Pavel tried this a while ago, but had to roll back [...] Got a bunch of test failures and such.

See:

Note that the test failures (in roachtest nightlies) could not be certainly pinned on the gRPC upgrade. So there is more to investigate.

However, at the very least this means we would benefit from manually running the nightly CI run on this dep bump PR, so as to collect the test results without impacting the master branch.

@knz
Copy link
Contributor

knz commented Nov 6, 2022

manually running the nightly CI run on this dep bump PR

in case you don't know this yet - you can manually trigger a TC target on any branch using the web UI.

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Nov 6, 2022

I'm pretty sure the kv/splits/nodes=3/quiesce=* failures were down to gRPC. I'd try to upgrade to 1.49 (previous versions have known bugs), and see if we can get that test to pass reliably. Might just need some tweaks in our integration, we didn't look closer (this was blocking 22.2).

@erikgrinaker
Copy link
Contributor

Is anyone actively working on the gRPC upgrade? If not, I can pick it up "soon".

@erikgrinaker
Copy link
Contributor

Bumping gRPC to v1.51.0 over in #93867, no longer see the roachtest failure we had with v1.49.0.

@knz
Copy link
Contributor

knz commented Aug 16, 2023

@rhu713 I think this was achieved by other means, right? do we need to keep this PR open?

@dt
Copy link
Member

dt commented Oct 17, 2023

bumped elsewhere

@dt dt closed this Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants