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

changefeedccl: more quickly notice new kafka partitions #32297

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@danhhz
Copy link
Contributor

danhhz commented Nov 14, 2018

We want to emit resolved timestamps to all partitions. Sarama caches
this information, so periocially refresh the cache.

Test is left as a TODO. We can't right now because we'd need to bump
sarama, but that's a bad idea while we're still actively working on
stability

For #28655.

Release note (bug fix): CHANGEFEEDs emitting into kafka more quickly
notice new partitions.

changefeedccl: more quickly notice new kafka partitions
We want to emit resolved timestamps to all partitions. Sarama caches
this information, so periocially refresh the cache.

Test is left as a TODO. We can't right now because we'd need to bump
sarama, but that's a bad idea while we're still actively working on
stability

For #28655.

Release note (bug fix): CHANGEFEEDs emitting into kafka more quickly
notice new partitions.

@danhhz danhhz requested a review from mrtracy Nov 14, 2018

@danhhz danhhz requested a review from cockroachdb/sql-ccl-prs as a code owner Nov 14, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 14, 2018

This change is Reviewable

@mrtracy
Copy link
Collaborator

mrtracy left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ccl/changefeedccl/sink.go, line 246 at r1 (raw file):

	if timeutil.Since(s.lastMetadataRefresh) > metadataRefreshMinDuration {
		topics := make([]string, 0, len(s.topics))
		for topic := range s.topics {

See, you don't need covariance, you can just do stuff like this!

@mrtracy
Copy link
Collaborator

mrtracy left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/ccl/changefeedccl/sink.go, line 246 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

See, you don't need covariance, you can just do stuff like this!

Oh, I see its not a covariance workaround, but rather that topics are a set. Ignore my comment!

@danhhz

This comment has been minimized.

Copy link
Contributor

danhhz commented Nov 15, 2018

TFTR!

bors r=mrtracy

craig bot pushed a commit that referenced this pull request Nov 15, 2018

Merge #32297
32297: changefeedccl: more quickly notice new kafka partitions r=mrtracy a=danhhz

We want to emit resolved timestamps to all partitions. Sarama caches
this information, so periocially refresh the cache.

Test is left as a TODO. We can't right now because we'd need to bump
sarama, but that's a bad idea while we're still actively working on
stability

For #28655.

Release note (bug fix): CHANGEFEEDs emitting into kafka more quickly
notice new partitions.

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 15, 2018

Build succeeded

@craig craig bot merged commit 4aec7f5 into cockroachdb:master Nov 15, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@danhhz danhhz deleted the danhhz:cdc_metadatarefresh branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment