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.

Sign up for GitHub

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

Jump to bottom

release-21.1: deps: upgrade gogoproto and cockroachdb/errors #77677

Closed
wants to merge 3 commits into from
Closed

release-21.1: deps: upgrade gogoproto and cockroachdb/errors #77677

wants to merge 3 commits into from

Conversation

knz
Copy link
Contributor

@knz knz commented Mar 11, 2022

Backports:

Release justification: low risk, high benefit changes to existing functionality

See individual PRs for details

@knz knz requested a review from a team March 11, 2022 15:46
@knz knz requested review from a team as code owners March 11, 2022 15:46
@knz knz requested a review from a team March 11, 2022 15:46
@knz knz requested review from a team as code owners March 11, 2022 15:46
@knz knz requested review from a team March 11, 2022 15:46
@knz knz requested a review from a team as a code owner March 11, 2022 15:46
@knz knz requested review from erikgrinaker and removed request for a team March 11, 2022 15:46
@blathers-crl
Copy link

blathers-crl bot commented Mar 11, 2022 •
edited by knz
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@knz knz requested review from adityamaru and removed request for a team March 11, 2022 15:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor

I'm not sure I'm entirely comfortable backporting these. What's the motivation?

@knz
Copy link
Contributor Author

knz commented Mar 11, 2022

Fixing #76571 as requested by @yuzefovich

@erikgrinaker
Copy link
Contributor

I feel like this pulls in too much additional stuff. The diff is 60k lines. Any way to strip this down to the bare minimum?

@knz
Copy link
Contributor Author

knz commented Mar 11, 2022

The diff is 60k lines.

Most of this is generated .pb.go, which was checked in in the 21.1 branch.

@erikgrinaker
Copy link
Contributor

Sure, but still -- this seems like a lot. Is it really worth it, especially when this will be out of support in a couple of months anyway?

Saw a bunch of conflict markers in here too, btw. E.g.:

<<<<<<< HEAD
        sum = "h1:4MY060fB1DLGMB/7MBTLnwQUY6+F09GEiz6SsrNqyzM=",
        version = "v1.23.0",
=======
        sum = "h1:bxAC2xTBsZGibn2RTntX0oH50xLsqy1OxA9tTL3p/lk=",
        version = "v1.26.0",
>>>>>>> bb6ebca751 (*: update to use gogoproto v1.3.)

knz and others added 3 commits March 11, 2022 16:13
@knz
*: update to use gogoproto v1.3.
8f2d4b7 
To achieve this version upgrade, some care needed to be taken:

- previously, `go.mod` was anchoring gogoproto to v1.2 and
  our own fork at github.com/cockroachdb/gogoproto.

  This fork was needed because the original v1.2 gogoproto
  had a bug, and our upstream PR was never merged:
  gogo/protobuf#529

  Thankfully, gogoproto did eventually fix that bug upstream (in a
  different way) so we do not need the custom patch anymore in v1.3.

  So this commit drops the anchor and lets the code start
  to depend on upstream gogoproto v1.3.1.

- doing so causes all `.pb.go` files to be updated, using the
  latest protobuf code generation changes in gogoproto v1.3.

  In particular, v1.3 extends its `Message` interface with a new
  method:

  ```go
  MarshalToSizedBuffer(data []byte) (int, error)
  ```

  This behaves like `MarshalTo()` but requires the buffer to have
  space already for at least `.Size()` more bytes.

- because of this new method, `.pb.go` code now requires all its
  recursively needed `Message` types to also implement the new method.

  In CockroachDB we have two "external" `.pb.go` dependencies and osome
  internal custom `Message` implementations (non-autogenerated):

  - `raftpb` for etcd's Raft protobuf messages
  - `errorspb` from `cockroachdb/errors`
  - the `sql/types.T` type.
  - the `util/hlc.ClockTimestamp` type.

  All these need to be upgraded to satisfy the new interface.

- to upgrade `sql/types.T`, this patch adds the missing method
  (by means of extending `util/protoutil/marshal.go` accordingly.

Because of the transitive minimum version dependencies in `etcd` and
`gogo/protobuf`, a couple of other packages were also updated:

- github.com/cockroachdb/errors
- github.com/gogo/protobuf
- github.com/golang/protobuf
- github.com/google/go-cmp
- github.com/google/uuid
- go.etcd.io/etcd/raft
- golang.org/x/net
- golang.org/x/sys
- golang.org/x/text
- golang.org/x/time

The changes seem fairly contained but the reviewer is invited to take
a cursory look nonetheless.

Release note: None
@knz @rail
deps: update errors and the sentry-go dependency
3c38743 
This updates the sentry SDK version from 0.6.1 to 0.12.0.

Release note: None

Co-authored-by: Rail Aliiev <rail@iqchoice.com>
@knz
deps: update cockroachdb/errors
0518910 
This upgrades to v1.9.0 and enhances Sentry reports as requested by
the SQL queries team.

Release justification: low risk, high benefit changes to existing functionality

Release note: None
@knz knz force-pushed the backport21.1-56147-77527 branch from abd2b98 to 0518910 Compare March 11, 2022 16:25
knz
knz commented Mar 11, 2022
View reviewed changes
Copy link
Contributor Author

@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.

this seems like a lot.

Excluding the generated files, we're looking at 13 modified go files. Most of this is test code adjustments; the only non-test code change adds protobuf marshal methods.

Is it really worth it, especially when this will be out of support in a couple of months anyway?

That's a good question. I'll let @yuzefovich and @jordanlewis chime in on this.

Saw a bunch of conflict markers in here too

That was just DEPS.bzl. This also auto-generated and I forgot to run the re-gen. Done now.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @erikgrinaker)

@erikgrinaker
Copy link
Contributor

Excluding the generated files, we're looking at 13 modified go files.

Sure, but the dependency bumps here aren't entirely trivial.

It's probably fine though -- we use these in 21.2 after all -- but I'd rather avoid the larger bumps if we can. We can't limit this to the sentry package alone, with some small error tweaks?

@knz
Copy link
Contributor Author

knz commented Mar 11, 2022

We can't limit this to the sentry package alone, with some small error tweaks?

That's what I started with, but some point in the line we had errors hard depend on gogoproto 1.3, hence the need to also backport #56147.

@yuzefovich
Copy link
Member

Is it really worth it, especially when this will be out of support in a couple of months anyway?

That's a good question. I'll let @yuzefovich and @jordanlewis chime in on this.

I think given the complications with the backport of #77525 it’s not worth it. Sorry for the trouble Raphael.

@knz
Copy link
Contributor Author

knz commented Mar 12, 2022

no worries!

@knz knz closed this Mar 12, 2022
@knz knz deleted the backport21.1-56147-77527 branch March 12, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgrinaker erikgrinaker Awaiting requested review from erikgrinaker erikgrinaker is a code owner automatically assigned from cockroachdb/test-eng

@adityamaru adityamaru Awaiting requested review from adityamaru

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knz @cockroach-teamcity @erikgrinaker @yuzefovich