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: allow per changefeed kafka quota config #118643

Merged
merged 1 commit into from Feb 9, 2024

Conversation

wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Feb 2, 2024

Previously, users were limited to setting a single kafka quota configuration for
cockroachdb which was then applied and restricting all changefeeds. This patch
introduces a new changefeed configuration option, allowing users to define
client id for different changefeeds, allowing users to specify different kafka
quota configurations for different changefeeds. To use it, users can specify a
unique client ID using kafka_sink_config and configure different quota
settings on kafka server based on
https://kafka.apache.org/documentation/#quotas.

CREATE CHANGEFEED FOR foo WITH kafka_sink_config='{"ClientID": "clientID1"}'

Fixes: #92290

Release note: kafka_sink_config now supports specifying a different client ID
for different changefeeds, enabling users to define distinct kafka quota
configurations for various changefeeds.

For any kafka versions >= V1_0_0_0 (KIP-190: Handle client-ids consistently
between clients and
brokers
),
any string can be used as client ID. For earlier kafka versions, clientID can
only contain characters [A-Za-z0-9._-] are acceptable.

For example,

CREATE CHANGEFEED FOR ... WITH kafka_sink_config='{"ClientID": "clientID1"}'

@wenyihu6 wenyihu6 requested a review from a team as a code owner February 2, 2024 15:33
@wenyihu6 wenyihu6 requested review from rharding6373 and removed request for a team February 2, 2024 15:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Feb 2, 2024

Splitted the roachtest work into another PR here - #118428. We can revisit what the best way to test this work is there.

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Feb 2, 2024

First commit comes from #117693.

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! Have a couple comments.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)


pkg/ccl/changefeedccl/sink_kafka.go line 838 at r2 (raw file):

	kafka.Producer.Flush.Frequency = time.Duration(c.Flush.Frequency)
	kafka.Producer.Flush.MaxMessages = c.Flush.MaxMessages
	kafka.ClientID = c.ClientID

Are there any restrictions on client IDs, e.g., length, invalid characters, that we need to validate?


pkg/ccl/changefeedccl/sink_test.go line 644 at r2 (raw file):

		cfg, err = getSaramaConfig(opts)
		require.NoError(t, err)
		require.NoError(t, cfg.Validate())

You should add an extra check to make sure that clientID1 is reflected in the cfg.

Is there also logic we should add toValidate to make sure the client id is sensical? See my other comment.

@wenyihu6 wenyihu6 self-assigned this Feb 6, 2024
@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Feb 7, 2024

pkg/ccl/changefeedccl/sink_kafka.go line 838 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Are there any restrictions on client IDs, e.g., length, invalid characters, that we need to validate?

Sarama provides an exported validation check in its library. I opened another PR to add this check #118890.

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Feb 7, 2024

pkg/ccl/changefeedccl/sink_test.go line 644 at r2 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

You should add an extra check to make sure that clientID1 is reflected in the cfg.

Is there also logic we should add toValidate to make sure the client id is sensical? See my other comment.

Done.

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:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

Previously, users were limited to setting a single kafka quota configuration for
cockroachdb which was then applied and restricting all changefeeds. This patch
introduces a new changefeed configuration option, allowing users to define
client id for different changefeeds, allowing users to specify different kafka
quota configurations for different changefeeds. To use it, users can specify a
unique client ID using `kafka_sink_config` and configure different quota
settings on kafka server based on
https://kafka.apache.org/documentation/#quotas.

```
CREATE CHANGEFEED FOR foo WITH kafka_sink_config='{"ClientID": "clientID1"}'
```

Note that Fixes: cockroachdb#92290

Release note: `kafka_sink_config` now supports specifying a different client ID
for different changefeeds, enabling users to define distinct kafka quota
configurations for various changefeeds.

For any kafka versions >= V1_0_0_0 ([KIP-190: Handle client-ids consistently
between clients and
brokers](https://cwiki.apache.org/confluence/display/KAFKA/KIP-190%3A+Handle+client-ids+consistently+between+clients+and+brokers)),
any string can be used as client ID. For earlier kafka versions, clientID can
only contain characters [A-Za-z0-9._-] are acceptable.

For example,
```
CREATE CHANGEFEED FOR ... WITH kafka_sink_config='{"ClientID": "clientID1"}'
```
@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Feb 9, 2024

TFTRs!!

bors r=rharding6373

@craig
Copy link
Contributor

craig bot commented Feb 9, 2024

Build succeeded:

@craig craig bot merged commit 353fded into cockroachdb:master Feb 9, 2024
9 checks passed
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: better handle kafka quotas
3 participants