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

c2c: refactor eventStream to track batch in seperate streamEventBatcher #106444

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

msbutler
Copy link
Collaborator

@msbutler msbutler commented Jul 7, 2023

This is a small refactor cleans up the streamLoop() in the eventStream and makes it easier to emit spanConfig protos for zone config replication.

Epic: none

Release note: None

@msbutler msbutler requested a review from stevendanna July 7, 2023 20:49
@msbutler msbutler self-assigned this Jul 7, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 7, 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.

@msbutler msbutler removed the request for review from stevendanna July 7, 2023 20:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@msbutler msbutler force-pushed the butler-producer-batch-refactor branch 5 times, most recently from a20f3bb to 4b2e2f0 Compare July 10, 2023 12:59
@msbutler
Copy link
Collaborator Author

unrelated flake

@msbutler msbutler marked this pull request as ready for review July 10, 2023 14:45
@msbutler msbutler requested a review from a team as a code owner July 10, 2023 14:45
@msbutler msbutler requested review from dt and stevendanna and removed request for a team and dt July 10, 2023 14:45
@msbutler msbutler force-pushed the butler-producer-batch-refactor branch 3 times, most recently from d006d86 to baf383c Compare July 14, 2023 21:08
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Thanks for doing these kinda cleanups.

I left some nits, but they are mostly just notes for the next time we touch this code.

"github.com/cockroachdb/cockroach/pkg/roachpb"
)

type batchManager struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I see manager I typically think of something that oversees a collection of a thing. But, let's not bikeshed the name.

Copy link
Collaborator Author

@msbutler msbutler Jul 16, 2023

Choose a reason for hiding this comment

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

refactored to streamEventBatcher. we can further bikeshed after this pr merges :)

pkg/ccl/streamingccl/streamproducer/event_stream.go Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@ import (
func ScanSST(
sst *kvpb.RangeFeedSSTable,
scanWithin roachpb.Span,
// TODO (msbutler): I think we can use a roachpb.kv instead, avoiding EncodeDecode roundtrip.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting.

@msbutler msbutler force-pushed the butler-producer-batch-refactor branch 3 times, most recently from 988f91e to 3bdbc20 Compare July 17, 2023 00:33
This is a small refactor cleans up the streamLoop() in the eventStream and
makes it easier to emit spanConfig protos for zone config replication.

Epic: none

Release note: None
@msbutler msbutler force-pushed the butler-producer-batch-refactor branch from 3bdbc20 to 8a2b868 Compare July 17, 2023 00:34
@msbutler msbutler changed the title c2c: refactor eventStream to track batch in seperate batchManager c2c: refactor eventStream to track batch in seperate streamEventBatcher Jul 17, 2023
@msbutler
Copy link
Collaborator Author

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Jul 17, 2023

Build succeeded:

@craig craig bot merged commit c89d586 into cockroachdb:master Jul 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.

4 participants