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

changefeedccl: add ignore_disable_changefeed_replication option #120255

Merged
merged 1 commit into from Mar 14, 2024

Conversation

andyyang890
Copy link
Collaborator

@andyyang890 andyyang890 commented Mar 12, 2024

Fixes #119891

Release note (enterprise change): A new boolean changefeed option
named ignore_disable_changefeed_replication has been added.
When set, the changefeed will not filter events even if CDC
filtering is configured, which is currently available via the
disable_changefeed_replication session variable,
sql.ttl.changefeed_replication.disabled cluster setting, and the
ttl_disable_changefeed_replication table storage parameter.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andyyang890 andyyang890 marked this pull request as ready for review March 12, 2024 01:52
@andyyang890 andyyang890 requested review from a team as code owners March 12, 2024 01:52
@andyyang890 andyyang890 requested review from herkolategan, renatolabs, jayshrivastava, rharding6373, a team and miraradeva and removed request for a team March 12, 2024 01:53
Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

Nice work! I have fairly minor comments.

Reviewed 3 of 4 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @herkolategan, @jayshrivastava, @miraradeva, and @renatolabs)


-- commits line 5 at r1:
I think the description of changefeedReplicationDisabled should be expanded to say that changefeeds with this option will not filter TTL delete events.


pkg/cmd/roachtest/tests/cdc_filtering.go line 271 at r1 (raw file):

		// Produce a canonical format that we can assert on. The format is of the
		// form: id@exp_at|<deleted> (before: id@exp_at).
		eventToString := func(before *state, after *state) string {

Can this function be moved into a shared helper function? It seems like there's duplicate logic here.

Copy link
Collaborator Author

@andyyang890 andyyang890 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 @herkolategan, @jayshrivastava, @miraradeva, @renatolabs, and @rharding6373)


-- commits line 5 at r1:

Previously, rharding6373 (Rachael Harding) wrote…

I think the description of changefeedReplicationDisabled should be expanded to say that changefeeds with this option will not filter TTL delete events.

Done.


pkg/cmd/roachtest/tests/cdc_filtering.go line 271 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Can this function be moved into a shared helper function? It seems like there's duplicate logic here.

Good call. I've refactored the code into a makeEventString function.

Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

:lgtm: Looks great!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890, @herkolategan, @jayshrivastava, @miraradeva, and @renatolabs)


pkg/cmd/roachtest/tests/cdc_filtering.go line 271 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Good call. I've refactored the code into a makeEventString function.

This was an even cleaner refactor than what I considered. Very nice! Thanks for doing that.


pkg/sql/ttl/ttlbase/ttl_helpers.go line 78 at r2 (raw file):

		"sql.ttl.changefeed_replication.disabled",
		"if true, deletes issued by TTL will not be replicated via changefeeds "+
			"(except changefeeds that have the ignore_disable_changefeed_replication option set)",

nit: add something like ", which will always replicate TTL deletes" to the text in parens for clarity.

Release note (enterprise change): A new boolean changefeed option
named `ignore_disable_changefeed_replication` has been added.
When set, the changefeed will not filter events even if CDC
filtering is configured, which is currently available via the
`disable_changefeed_replication` session variable,
`sql.ttl.changefeed_replication.disabled` cluster setting, and the
`ttl_disable_changefeed_replication` table storage parameter.
Copy link
Collaborator Author

@andyyang890 andyyang890 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 (and 1 stale) (waiting on @herkolategan, @jayshrivastava, @miraradeva, @renatolabs, and @rharding6373)


pkg/cmd/roachtest/tests/cdc_filtering.go line 271 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

This was an even cleaner refactor than what I considered. Very nice! Thanks for doing that.

Thanks!


pkg/sql/ttl/ttlbase/ttl_helpers.go line 78 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

nit: add something like ", which will always replicate TTL deletes" to the text in parens for clarity.

Updated, hopefully it's more clear now.

@andyyang890
Copy link
Collaborator Author

TFTR!

bors r=rharding6373

@andyyang890
Copy link
Collaborator Author

Seems like it didn't get added to the bors queue

bors r=rharding6373

@craig
Copy link
Contributor

craig bot commented Mar 14, 2024

@craig craig bot merged commit 915469c into cockroachdb:master Mar 14, 2024
18 of 19 checks passed
@andyyang890 andyyang890 deleted the cdc_ignore_filtering branch March 14, 2024 19:31
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.

changefeedccl: allow individual changefeed jobs to configure filtering
3 participants