You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/18 15:15:07 UTC
[GitHub] [kafka] mimaison opened a new pull request, #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
mimaison opened a new pull request, #12536:
URL: https://github.com/apache/kafka/pull/12536
Cache the Kafka cluster Id once it has been retrieved to avoid creating many Admin clients at startup.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12536:
URL: https://github.com/apache/kafka/pull/12536
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12536:
URL: https://github.com/apache/kafka/pull/12536#discussion_r952685256
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java:
##########
@@ -409,6 +412,13 @@ public String groupId() {
return null;
}
+ public String kafkaClusterId() {
+ if (kafkaClusterId == null) {
+ kafkaClusterId = ConnectUtils.lookupKafkaClusterId(this);
+ }
+ return kafkaClusterId;
+ }
Review Comment:
Yeah, was wondering about the fallout on tests. This seems fine for now 👍
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12536:
URL: https://github.com/apache/kafka/pull/12536#discussion_r952686683
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java:
##########
@@ -306,6 +310,35 @@ protected static ConfigDef baseConfigDef() {
.withClientSslSupport();
}
+ private String kafkaClusterId;
+
+ public static String lookupKafkaClusterId(WorkerConfig config) {
Review Comment:
We may want to add a comment or even deprecation to this method to prevent people from adding new call sites for it and to note that it should be downgraded in visibility once our testing migration is complete.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12536:
URL: https://github.com/apache/kafka/pull/12536#discussion_r952748560
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java:
##########
@@ -306,6 +310,35 @@ protected static ConfigDef baseConfigDef() {
.withClientSslSupport();
}
+ private String kafkaClusterId;
+
+ public static String lookupKafkaClusterId(WorkerConfig config) {
Review Comment:
Thanks! I opened https://issues.apache.org/jira/browse/KAFKA-14176 to address it, so I'll merge like this.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12536:
URL: https://github.com/apache/kafka/pull/12536#discussion_r950692858
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java:
##########
@@ -409,6 +412,13 @@ public String groupId() {
return null;
}
+ public String kafkaClusterId() {
+ if (kafkaClusterId == null) {
+ kafkaClusterId = ConnectUtils.lookupKafkaClusterId(this);
+ }
+ return kafkaClusterId;
+ }
Review Comment:
Should we move the `lookupKafkaClusterId` logic from `ConnectUtils` to this class, so that nobody accidentally starts using the variant that creates and discards an admin client once per call?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #12536: KAFKA-14160: Streamline clusterId retrieval in Connect
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12536:
URL: https://github.com/apache/kafka/pull/12536#discussion_r952552570
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java:
##########
@@ -409,6 +412,13 @@ public String groupId() {
return null;
}
+ public String kafkaClusterId() {
+ if (kafkaClusterId == null) {
+ kafkaClusterId = ConnectUtils.lookupKafkaClusterId(this);
+ }
+ return kafkaClusterId;
+ }
Review Comment:
That's a good idea. I've move `lookupKafkaClusterId` to `WorkerConfig`. We should be able to make this method not public by updating some of the tests but I've not done that since some of the callers are still using Easymock/Powermock so they are being updated.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org