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

streamingccl: support sst event in random stream client #85392

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

gh-casper
Copy link
Contributor

@gh-casper gh-casper commented Aug 1, 2022

Previously random stream client lacks test coverage
for AddSSTable operation, this PR enables random stream
client to generate and validate random SSTableEvents.

This PR also cleans up the InterceptableStreamClient
to avoid unnecessary abstraction, making code cleaner.

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

Release note : None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

func (r *randomEventGenerator) generateNewEvent() streamingccl.Event {
var event streamingccl.Event
if r.numKVEventsSinceLastResolved == r.config.kvsPerCheckpoint &&
r.numKVEventsSinceLastResolved == r.config.sstsPerCheckpoint {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be numSSTEventsSinceLastResolved?

@gh-casper gh-casper force-pushed the random-stream-client branch from 83e1ff8 to 6946cc1 Compare August 12, 2022 02:47
Copy link
Contributor Author

@gh-casper gh-casper left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy, @samiskin, and @stevendanna)


pkg/ccl/streamingccl/streamclient/random_stream_client.go line 238 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

Should this be numSSTEventsSinceLastResolved?

Done. I also changed the logic to generate SST events -- using probability instead of checking num of events generated which may cause bug and is not very intuitive.

Decided to move the delrange x random_stream_client in another PR.

@gh-casper gh-casper requested a review from stevendanna August 12, 2022 02:52
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 adding this! I gave it a once over and it looks good to me.

WriteTS: sst.WriteTS,
})
default:
panic("unsopported event type")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
panic("unsopported event type")
panic("unsupported event type")

@gh-casper gh-casper force-pushed the random-stream-client branch 3 times, most recently from 406e697 to aa2d95b Compare August 24, 2022 05:27
Previously random stream client lacks test coverage
for AddSSTable operation, this PR enables random stream
client to generate and validate random SSTableEvents.

This PR also cleans up the InterceptableStreamClient
to avoid unnecessary abstraction, making code cleaner.

Release note : None

Release justification: low risk, high benefit changes
to existing functionality
@gh-casper gh-casper force-pushed the random-stream-client branch from aa2d95b to 7a8f501 Compare August 24, 2022 12:46
@gh-casper
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@gh-casper
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Already running a review

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Aug 24, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2022

Build succeeded:

@craig craig bot merged commit af9d88c into cockroachdb:master Aug 25, 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