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

update pubsub dependency #96449

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

shermanCRL
Copy link
Contributor

@shermanCRL shermanCRL commented Feb 2, 2023

Motivated by an apparent shortcoming in a dependency (internal conversation).

This brings in updates for sub dependencies, here are some diffs:

Jira: none
Epic: none

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@shermanCRL shermanCRL marked this pull request as draft February 2, 2023 18:17
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch 2 times, most recently from 78989dd to da18080 Compare February 6, 2023 18:58
@knz
Copy link
Contributor

knz commented Feb 6, 2023

golang/net@a158d28...f15817d

LGTM - this change also opens a door for an improvement in CockroachDB, that of propagating context cancellation through RPC dials. We might notice stability improvements as a result.

golang/sync@886fb93...v0.1.0

LGTM

golang/time@1f47c86...v0.1.0

This one would need an extra pair of eyes from our KV and storage team, as we're relying on rate.Limiter in a few places and this is changing semantics in this dep upgrade.

protocolbuffers/protobuf-go@v1.28.0...v1.28.1

LGTM

@shermanCRL shermanCRL marked this pull request as ready for review February 7, 2023 16:15
@shermanCRL shermanCRL requested a review from a team as a code owner February 7, 2023 16:15
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from da18080 to 7b28ab1 Compare February 7, 2023 16:23
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

My approval is only valid for the specific things i've reviewed.

@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from 7b28ab1 to b9ce872 Compare February 8, 2023 19:07
@shermanCRL
Copy link
Contributor Author

googleapis/google-api-go-client@v0.80.0...v0.103.0 is a large diff, but appears to be almost entirely boilerplate and codegen. I don’t notice any relevant feature changes in there.

@shermanCRL
Copy link
Contributor Author

google/go-cmp@v0.5.8...v0.5.9 looks like uninteresting changes.

@shermanCRL
Copy link
Contributor Author

census-instrumentation/opencensus-go@v0.23.0...v0.24.0 Looks like mostly optimizations, and one fix that appears to prevent leaking / hanging goroutines (which was the motivation of this overall dep update in the first place cc @ZhouXing19).

@shermanCRL
Copy link
Contributor Author

googleapis/go-genproto@f39f71e...67e5cbc appears to be all codegen and dependency updates, I don’t detect meaningful changes in there.

@nvanbenschoten
Copy link
Member

golang/time@1f47c86...v0.1.0

LGTM

@shermanCRL shermanCRL requested a review from adityamaru February 9, 2023 04:00
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from b9ce872 to 4d167b1 Compare February 9, 2023 15:45
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from 4d167b1 to d78ba57 Compare February 16, 2023 21:37
@shermanCRL
Copy link
Contributor Author

shermanCRL commented Feb 16, 2023

Because googleapis/google-cloud-go@kms/v1.1.0...kms/v1.6.0 and googleapis/google-cloud-go@pubsub/v1.16.0...pubsub/v1.28.0 are:

  • Difficult to the reasonably review
  • First-party

...I think we can reasonably make the call that we trust it, and we will depend on our tests to detect regressions. @adityamaru @knz agree?

@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from d78ba57 to b0ced58 Compare February 17, 2023 01:14
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from b0ced58 to 64dee1a Compare February 22, 2023 18:22
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from 64dee1a to f0632bc Compare February 23, 2023 15:06
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from f0632bc to 604deea Compare February 24, 2023 00:17
@shermanCRL shermanCRL requested a review from a team as a code owner February 24, 2023 00:17
@blathers-crl
Copy link

blathers-crl bot commented Feb 24, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch 2 times, most recently from 6dec397 to 99efc5b Compare February 27, 2023 14:22
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from 99efc5b to b0ea9cb Compare March 2, 2023 20:50
Includes several sub-dependency updates, see cockroachdb#96449

Note removal of redirect for google.golang.org/api v0.80.0 (bottom of go.mod). I believe that fork existed to get upstream fixes earlier, and the later version update includes that fix.
@shermanCRL shermanCRL force-pushed the pubsub-dependency-update branch from b0ea9cb to e846fce Compare March 7, 2023 22:28
@shermanCRL
Copy link
Contributor Author

shermanCRL commented Mar 7, 2023

Removed redirect:
replace google.golang.org/api v0.80.0 => github.com/cockroachdb/google-api-go-client v0.80.1-0.20221117193156-6a9f7150cb93

...as this fork by @rhu713 was an eager backport of an upstream change. That change is in the later version in this PR.

@shermanCRL
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 8, 2023

Build succeeded:

@craig craig bot merged commit cd0ff71 into cockroachdb:master Mar 8, 2023
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