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 protobuf-related deps #33329

Merged
merged 1 commit into from
Jan 3, 2019
Merged

Conversation

RaduBerinde
Copy link
Member

This change updates the protobuf deps and makes various fixes. Most
fixes are related to new XXX_ fields. In particular, the new fields
interfere with JSON marshaling and printing with %v.

Note that the new fields could be omitted but that is undesirable:
XXX_NoUnkeyedLiteral is a good idea and there would be a performance
cost associated with removing XXX_sizecache.

Some more info on important protobuf changes here:
https://groups.google.com/forum/#!topic/golang-nuts/F5xFHTfwRnY

Change lists:
gogo/protobuf@1adfc12...936d59a
googleapis/go-genproto@f676e0f...a1fde74

Note that gogo/protobuf was forked under cockroachdb, and I made a fix
in our fork. This commit should be reviewed:
gogo/protobuf@d0154be

Informs #30774.

Release note: None

I ran some before/after tests with a KV workload, didn't see any difference beyond the usual variations.

@RaduBerinde RaduBerinde requested review from dt, benesch, tbg, petermattis and a team December 21, 2018 18:22
@RaduBerinde RaduBerinde requested a review from a team as a code owner December 21, 2018 18:22
@RaduBerinde RaduBerinde requested review from a team December 21, 2018 18:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm: but I had some minor comments on the gogoproto commit.

Reviewed 90 of 90 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


Makefile, line 1087 at r1 (raw file):

	gofmt -s -w $(GO_SOURCES)
	touch $@
		# -e 's!cockroachdb/cockroach/pkg/(prometheus/client_model)!\1/go!g' \

?


pkg/roachpb/errors.go, line 530 at r1 (raw file):

		"previous write with future timestamp %s within uncertainty interval `t <= %v`; "+
		"observed timestamps: %s",
		e.ReadTimestamp, e.ExistingTimestamp, e.MaxTimestamp, ts.String())

It's a shame that these aren't omitted when they're empty via some default stringer. Then again they probably are, but we have the default stringer disabled? Anyway, this is fine.


pkg/server/node.go, line 376 at r1 (raw file):

	ctx := n.AnnotateCtx(context.Background())
	if err := n.stores.OnClusterVersionChange(ctx, cv); err != nil {
		log.Fatal(ctx, errors.Wrapf(err, "updating cluster version to %v", cv))

What forced you to make these changes?

@RaduBerinde RaduBerinde force-pushed the update-proto branch 2 times, most recently from 599cade to fcf2476 Compare January 2, 2019 20:01
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Thank you! Updated.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


Makefile, line 1087 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

?

Oops, leftover, removed. We no longer need this one.


pkg/server/node.go, line 376 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

What forced you to make these changes?

I think that while I was fixing stuff I hit this panic and the message didn't look good (I don't think it has a Stringer implemented).

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@RaduBerinde RaduBerinde force-pushed the update-proto branch 4 times, most recently from 0de8e7a to 0ed2041 Compare January 3, 2019 11:44
This change updates the protobuf deps and makes various fixes. Most
fixes are related to new `XXX_` fields. In particular, the new fields
interfere with JSON marshaling and printing with `%v`.

Note that the new fields could be omitted but that is undesirable:
`XXX_NoUnkeyedLiteral` is a good idea and there would be a performance
cost associated with removing `XXX_sizecache`.

Some more info on important protobuf changes here:
https://groups.google.com/forum/#!topic/golang-nuts/F5xFHTfwRnY

Change lists:
gogo/protobuf@1adfc12...936d59a
googleapis/go-genproto@f676e0f...a1fde74

Note that gogo/protobuf was forked under cockroachdb, and I made a fix
in our fork. This commit should be reviewed:
cockroachdb/gogoproto@d26e6ac

Informs cockroachdb#30774.

Release note: None
@RaduBerinde
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Jan 3, 2019
33329: *: update protobuf-related deps r=RaduBerinde a=RaduBerinde

This change updates the protobuf deps and makes various fixes. Most
fixes are related to new `XXX_` fields. In particular, the new fields
interfere with JSON marshaling and printing with `%v`.

Note that the new fields could be omitted but that is undesirable:
`XXX_NoUnkeyedLiteral` is a good idea and there would be a performance
cost associated with removing `XXX_sizecache`.

Some more info on important protobuf changes here:
https://groups.google.com/forum/#!topic/golang-nuts/F5xFHTfwRnY

Change lists:
gogo/protobuf@1adfc12...936d59a
googleapis/go-genproto@f676e0f...a1fde74

Note that gogo/protobuf was forked under cockroachdb, and I made a fix
in our fork. This commit should be reviewed:
gogo/protobuf@d0154be

Informs #30774.

Release note: None

I ran some before/after tests with a KV workload, didn't see any difference beyond the usual variations.

Co-authored-by: Radu Berinde <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jan 3, 2019

Build succeeded

@craig craig bot merged commit db4a8a1 into cockroachdb:master Jan 3, 2019
@tbg
Copy link
Member

tbg commented Jan 3, 2019 via email

@RaduBerinde RaduBerinde deleted the update-proto branch January 3, 2019 14:15
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.

3 participants