You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "hugofriant (via GitHub)" <gi...@apache.org> on 2024/11/20 10:42:07 UTC
[PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
hugofriant opened a new pull request, #11599:
URL: https://github.com/apache/iceberg/pull/11599
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852513028
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -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";
Review Comment:
It was the first proposal, but I proposed to keep the consumer-group semantic. OK for just group.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1850499181
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -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";
Review Comment:
As suggested before, I would use `consumer-group-id-prefix`.
##########
docs/docs/kafka-connect.md:
##########
@@ -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` |
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852504069
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -180,6 +181,12 @@ private static ConfigDef newConfigDef() {
DEFAULT_CONTROL_TOPIC,
Importance.MEDIUM,
"Name of the control topic");
+ configDef.define(
+ CONTROL_GROUP_ID_PREFIX_PROP,
+ ConfigDef.Type.STRING,
+ DEFAULT_CONTROL_GROUP_PREFIX,
+ Importance.LOW,
+ "Prefix of the control consumer group, should not be set under normal conditions");
Review Comment:
I think it is reasonable to set this, so I'd make this `Prefix of the control consumer group`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "hugofriant (via GitHub)" <gi...@apache.org>.
hugofriant commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2504053477
Hello @bryanck, can you merge the PR ? Thank you !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2491677885
@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 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2493915018
Looks good to me, thanks @hugofriant for the contribution, and @jbonofre for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852514071
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -359,6 +366,9 @@ public String controlTopic() {
return getString(CONTROL_TOPIC_PROP);
}
+ public String prefixControlConsumerGroup() {
Review Comment:
Again, it was the first proposal, I asked for consumer to clear with the kafka semantic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2491759742
Thanks for the PR @hugofriant , I had just a few naming nits
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852505064
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -359,6 +366,9 @@ public String controlTopic() {
return getString(CONTROL_TOPIC_PROP);
}
+ public String prefixControlConsumerGroup() {
Review Comment:
Rename to `controlGroupIdPrefix`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852521863
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -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";
Review Comment:
I think it's OK to shorted it, e.g. Kafka's consumer property is `group.id`.
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -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";
Review Comment:
I think it's OK to shorten it, e.g. Kafka's consumer property is `group.id`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "hugofriant (via GitHub)" <gi...@apache.org>.
hugofriant commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2493945187
I had to fix the format @bryanck. Could you relaunch the workflows ? Thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on code in PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#discussion_r1852497963
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/IcebergSinkConfig.java:
##########
@@ -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";
Review Comment:
We should name this `iceberg.control.group-id-prefix` which is more consistent (and shorter).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11599:
URL: https://github.com/apache/iceberg/pull/11599#issuecomment-2491704752
@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 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Kafka Connect: Add config to prefix the control consumer group [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck merged PR #11599:
URL: https://github.com/apache/iceberg/pull/11599
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org