Kafka Connect: Add config to prefix the control consumer group #11599
Conversation
docs/docs/kafka-connect.md
Outdated
| @@ -77,6 +77,7 @@ for exactly-once semantics. This requires Kafka 2.5 or later. | |||
| | iceberg.table.\<table name\>.partition-by | Comma-separated list of partition fields to use when creating the table | | |||
| | iceberg.table.\<table name\>.route-regex | The regex used to match a record's `routeField` to a table | | |||
| | iceberg.control.topic | Name of the control topic, default is `control-iceberg` | | |||
| | iceberg.control.group-id-prefix | Prefix for the control consuler group, default is `cg-control` | | |||
There was a problem hiding this comment.
There's a type here: I guess you mean consumer (not consuler :)).
I would rather name this property iceberg.control.consumer-group-id-prefix to keep the kafka semantic.
| @@ -80,6 +80,7 @@ public class IcebergSinkConfig extends AbstractConfig { | |||
| private static final String TABLES_SCHEMA_CASE_INSENSITIVE_PROP = | |||
| "iceberg.tables.schema-case-insensitive"; | |||
| private static final String CONTROL_TOPIC_PROP = "iceberg.control.topic"; | |||
| private static final String CONTROL_GROUP_PREFIX_PROP = "iceberg.control.group-id-prefix"; | |||
There was a problem hiding this comment.
As suggested before, I would use consumer-group-id-prefix.
|
@bryanck @rdblue I reviewed this PR and worked with @hugofriant on nit changes. I think this PR is ready to be merged 😄 Can you please take a look as well ? Thanks ! |
|
@RussellSpitzer I reviewed this PR and worked with @hugofriant on nit changes. I think this PR is ready to be merged 😄 Can you also please take a look (and merge) ? Thanks ! |
| @@ -80,6 +80,7 @@ public class IcebergSinkConfig extends AbstractConfig { | |||
| private static final String TABLES_SCHEMA_CASE_INSENSITIVE_PROP = | |||
| "iceberg.tables.schema-case-insensitive"; | |||
| private static final String CONTROL_TOPIC_PROP = "iceberg.control.topic"; | |||
| private static final String CONTROL_GROUP_ID_PREFIX_PROP = "iceberg.control.consumer-group-id-prefix"; | |||
There was a problem hiding this comment.
We should name this iceberg.control.group-id-prefix which is more consistent (and shorter).
There was a problem hiding this comment.
It was the first proposal, but I proposed to keep the consumer-group semantic. OK for just group.
There was a problem hiding this comment.
I think it's OK to shorten it, e.g. Kafka's consumer property is group.id.
| ConfigDef.Type.STRING, | ||
| DEFAULT_CONTROL_GROUP_PREFIX, | ||
| Importance.LOW, | ||
| "Prefix of the control consumer group, should not be set under normal conditions"); |
There was a problem hiding this comment.
I think it is reasonable to set this, so I'd make this Prefix of the control consumer group
| @@ -359,6 +366,9 @@ public String controlTopic() { | |||
| return getString(CONTROL_TOPIC_PROP); | |||
| } | |||
|
|
|||
| public String prefixControlConsumerGroup() { | |||
There was a problem hiding this comment.
Rename to controlGroupIdPrefix
There was a problem hiding this comment.
Again, it was the first proposal, I asked for consumer to clear with the kafka semantic.
|
Thanks for the PR @hugofriant , I had just a few naming nits |
I would like to control the name of the control consumer group to have more fine grained accesses when deploying the Iceberg Sink connector on my infrastructure.
This PR aims to replace the DEFAULT_CONTROL_GROUP_PREFIX by a configurable variable