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

util/log: rework eventpb so that log does not depend on it #85643

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Aug 4, 2022

This commit reworks some code generation and dependencies such that eventpb
now depends on logpb and log no longer needs to depend on eventpb. This is
important so that we can import protocol buffers into eventpb from other
protobuf packages like descpb and hlc which themselves import util/log.

Realistically it's bad that those packages import log, but this was easier
to break in a sense than was that, and this is also a sane depedendency to
not have.

Release note: None

@ajwerner ajwerner requested a review from a team August 4, 2022 23:02
@ajwerner ajwerner requested review from a team as code owners August 4, 2022 23:02
@ajwerner ajwerner requested a review from a team August 4, 2022 23:02
@ajwerner ajwerner requested a review from a team as a code owner August 4, 2022 23:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch from 52c98bc to dd9d84b Compare August 4, 2022 23:20
@ajwerner ajwerner requested a review from a team August 4, 2022 23:20
@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch from dd9d84b to 7eae1a3 Compare August 4, 2022 23:43
@ajwerner ajwerner requested a review from knz August 5, 2022 01:45
@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch 2 times, most recently from d63ce42 to c69df3e Compare August 5, 2022 03:56
@ajwerner ajwerner requested review from a team and msbutler August 5, 2022 03:56
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.

:lgtm:

Reviewed 66 of 66 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @msbutler)


pkg/util/log/eventpb/events.go line 82 at r1 (raw file):

var _ EventWithCommonSchemaChangePayload = (*FinishSchemaChange)(nil)
var _ EventWithCommonSchemaChangePayload = (*ReverseSchemaChange)(nil)
var _ EventWithCommonSchemaChangePayload = (*FinishSchemaChangeRollback)(nil)

Where are these interface assertions moved to?

@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch from c69df3e to 523de4c Compare August 5, 2022 14:58
@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 5, 2022

Thanks for the review!

Where are these interface assertions moved to?

Removed by accident. Added them back.

@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch from 523de4c to ade283c Compare August 5, 2022 15:41
This commit reworks some code generation and dependencies such that eventpb
now depends on logpb and log no longer needs to depend on eventpb. This is
important so that we can import protocol buffers into eventpb from other
protobuf packages like descpb and hlc which themselves import util/log.

Realistically it's bad that those packages import log, but this was easier
to break in a sense than was that, and this is also a sane depedendency to
not have.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/break-log-eventpb-dep branch from ade283c to 847fddd Compare August 5, 2022 16:48
@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 5, 2022

bors r=knz

@craig
Copy link
Contributor

craig bot commented Aug 5, 2022

Build succeeded:

@craig craig bot merged commit d4f2c41 into cockroachdb:master Aug 5, 2022
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