Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
mimaison merged PR #14206: URL: https://github.com/apache/kafka/pull/14206 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-2010532534 @mimaison could you help merge this if this looks good to 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-1954880081 Thanks @mimaison, looks like this is because I split out the TopicsImageTest changes into a separate PR. The migration tests depend on the `DELTA1_RECORDS` defined there so I moved over changes from TopicsImageTest as well. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
mimaison commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-1952110096 There's quite a few failures related to ZkMigration in the last CI run: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14206/22/testReport/ -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-193746 @mimaison @mumrah I moved unrelated test changes over to https://github.com/apache/kafka/pull/15373. The latest commit [1c1eb7d](https://github.com/apache/kafka/pull/15373/commits/1c1eb7d1a866c56188a97f6fa41940933e456c03) addresses the comments left here (mostly imports) -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-1944388740 @mimaison the PR was originally meant to introduce additional test changes, I believe @mumrah renamed it after I added the migration fix. I'll move the tests out and apply your suggestions, 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
mimaison commented on code in PR #14206:
URL: https://github.com/apache/kafka/pull/14206#discussion_r1489636848
##
core/src/main/scala/kafka/zk/migration/ZkConfigMigrationClient.scala:
##
@@ -226,8 +226,8 @@ class ZkConfigMigrationClient(
val (migrationZkVersion, responses) =
zkClient.retryMigrationRequestsUntilConnected(requests, state)
if (responses.head.resultCode.equals(Code.NONODE)) {
-// Not fatal.
-error(s"Did not delete $configResource since the node did not exist.")
+// Not fatal. This is expected in the case this is a topic config and
we delete the topic (KAFKA-16206)
Review Comment:
We usually don't include mention to Jiras. The explanation seems sufficient.
##
metadata/src/test/java/org/apache/kafka/image/ClientQuotasImageTest.java:
##
@@ -17,6 +17,7 @@
package org.apache.kafka.image;
+import java.util.Collections;
Review Comment:
Can we move this with the other `java.util` imports?
##
metadata/src/test/java/org/apache/kafka/image/ClusterImageTest.java:
##
@@ -64,6 +71,12 @@ public class ClusterImageTest {
final static ClusterImage IMAGE2;
+static final List DELTA2_RECORDS;
+
+static final ClusterDelta DELTA2;
+
+final static ClusterImage IMAGE3;
Review Comment:
nit: `static final` is the preferred syntax
##
metadata/src/test/java/org/apache/kafka/image/ClusterImageTest.java:
##
@@ -17,6 +17,11 @@
package org.apache.kafka.image;
+import org.apache.kafka.common.metadata.RegisterBrokerRecord;
Review Comment:
Can we move these imports with the other `org.apache.kafka.common.metadata`
imports below?
##
metadata/src/test/java/org/apache/kafka/image/ConfigurationsImageTest.java:
##
@@ -17,6 +17,7 @@
package org.apache.kafka.image;
+import java.util.Collections;
Review Comment:
Let's move this below
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-1942168244 @mimaison tagging you again just in case you have time to take a look @mumrah I've added the logging change in the meantime - are we okay merging in the next week if we don't get a response? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16206 Fix unnecessary topic config deletion during ZK migration [kafka]
ahuang98 commented on PR #14206: URL: https://github.com/apache/kafka/pull/14206#issuecomment-1930876640 Is there a downside to having `deleteTopic` in `ZkTopicMigrationClient` not delete configs? Otherwise changing the logging level seems okay to me. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
