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/12/03 01:05:59 UTC
[GitHub] [kafka] mumrah opened a new pull request, #12946: KAFKA-14427 ZK client support for migrations
mumrah opened a new pull request, #12946:
URL: https://github.com/apache/kafka/pull/12946
TODO
--
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] cmccabe commented on a diff in pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12946:
URL: https://github.com/apache/kafka/pull/12946#discussion_r1041287139
##########
core/src/main/scala/kafka/server/ZkAdminManager.scala:
##########
@@ -54,6 +54,19 @@ import org.apache.kafka.common.utils.Sanitizer
import scala.collection.{Map, mutable, _}
import scala.jdk.CollectionConverters._
+object ZkAdminManager {
+ def clientQuotaPropsToDoubleMap(props: Map[String, String]): Map[String, Double] = {
Review Comment:
can we have a unit test for this function?
--
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] dengziming commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
dengziming commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1337436123
Thanks for this PR @mumrah , I have added this to my review backlog and will take a look this week.
--
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] cmccabe commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1339779087
* need to add idempotence stuff (handle error where we create something and it already exists, or delete something and it's already gone)
--
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] mumrah commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1340102935
@cmccabe I've added a large constant (10M) to the controller epoch when persisting it in ZK. I think this workaround should allow us to move forward with integration/system testing prior to getting KAFKA-14436 finished.
WDYT about adding the idempotent stuff as a follow-on PR?
--
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] cmccabe commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1340220864
> WDYT about adding the idempotent stuff as a follow-on PR?
+1
--
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] mumrah commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1341224855
@dengziming yes, there will be more PRs coming :). We are trying to get the basics for migration in place for 3.4 as an "early access" feature. If you find issues or have questions after this is merged, feel free to comment on this PR or file a JIRA.
--
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] cmccabe commented on a diff in pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12946:
URL: https://github.com/apache/kafka/pull/12946#discussion_r1041288418
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/ZkMigrationLeadershipState.java:
##########
@@ -100,26 +106,30 @@ public boolean zkMigrationComplete() {
@Override
public String toString() {
return "ZkMigrationLeadershipState{" +
- "kraftControllerId=" + kraftControllerId +
- ", kraftControllerEpoch=" + kraftControllerEpoch +
- ", kraftMetadataOffset=" + kraftMetadataOffset +
- ", kraftMetadataEpoch=" + kraftMetadataEpoch +
- ", lastUpdatedTimeMs=" + lastUpdatedTimeMs +
- ", migrationZkVersion=" + migrationZkVersion +
- ", controllerZkVersion=" + controllerZkVersion +
- '}';
+ "kraftControllerId=" + kraftControllerId +
+ ", kraftControllerEpoch=" + kraftControllerEpoch +
+ ", kraftMetadataOffset=" + kraftMetadataOffset +
+ ", kraftMetadataEpoch=" + kraftMetadataEpoch +
+ ", lastUpdatedTimeMs=" + lastUpdatedTimeMs +
+ ", migrationZkVersion=" + migrationZkVersion +
+ ", controllerZkVersion=" + controllerZkVersion +
+ '}';
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ZkMigrationLeadershipState that = (ZkMigrationLeadershipState) o;
- return kraftControllerId == that.kraftControllerId && kraftControllerEpoch == that.kraftControllerEpoch && kraftMetadataOffset == that.kraftMetadataOffset && kraftMetadataEpoch == that.kraftMetadataEpoch && lastUpdatedTimeMs == that.lastUpdatedTimeMs && migrationZkVersion == that.migrationZkVersion && controllerZkVersion == that.controllerZkVersion;
+ return kraftControllerId == that.kraftControllerId && kraftControllerEpoch == that.kraftControllerEpoch
Review Comment:
can we do one line per comparison? a little hard to read otherwise
##########
metadata/src/main/java/org/apache/kafka/metadata/migration/ZkMigrationLeadershipState.java:
##########
@@ -100,26 +106,30 @@ public boolean zkMigrationComplete() {
@Override
public String toString() {
return "ZkMigrationLeadershipState{" +
- "kraftControllerId=" + kraftControllerId +
- ", kraftControllerEpoch=" + kraftControllerEpoch +
- ", kraftMetadataOffset=" + kraftMetadataOffset +
- ", kraftMetadataEpoch=" + kraftMetadataEpoch +
- ", lastUpdatedTimeMs=" + lastUpdatedTimeMs +
- ", migrationZkVersion=" + migrationZkVersion +
- ", controllerZkVersion=" + controllerZkVersion +
- '}';
+ "kraftControllerId=" + kraftControllerId +
+ ", kraftControllerEpoch=" + kraftControllerEpoch +
+ ", kraftMetadataOffset=" + kraftMetadataOffset +
+ ", kraftMetadataEpoch=" + kraftMetadataEpoch +
+ ", lastUpdatedTimeMs=" + lastUpdatedTimeMs +
+ ", migrationZkVersion=" + migrationZkVersion +
+ ", controllerZkVersion=" + controllerZkVersion +
+ '}';
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ZkMigrationLeadershipState that = (ZkMigrationLeadershipState) o;
- return kraftControllerId == that.kraftControllerId && kraftControllerEpoch == that.kraftControllerEpoch && kraftMetadataOffset == that.kraftMetadataOffset && kraftMetadataEpoch == that.kraftMetadataEpoch && lastUpdatedTimeMs == that.lastUpdatedTimeMs && migrationZkVersion == that.migrationZkVersion && controllerZkVersion == that.controllerZkVersion;
+ return kraftControllerId == that.kraftControllerId && kraftControllerEpoch == that.kraftControllerEpoch
+ && kraftMetadataOffset == that.kraftMetadataOffset && kraftMetadataEpoch == that.kraftMetadataEpoch
+ && lastUpdatedTimeMs == that.lastUpdatedTimeMs && migrationZkVersion == that.migrationZkVersion
+ && controllerZkVersion == that.controllerZkVersion;
}
@Override
public int hashCode() {
- return Objects.hash(kraftControllerId, kraftControllerEpoch, kraftMetadataOffset, kraftMetadataEpoch, lastUpdatedTimeMs, migrationZkVersion, controllerZkVersion);
+ return Objects.hash(kraftControllerId, kraftControllerEpoch, kraftMetadataOffset, kraftMetadataEpoch,
Review Comment:
can we do one line per item
--
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] mumrah commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1336208057
@dengziming
--
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] cmccabe commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1339772134
We do need a temporary workaround for KAFKA-14436. We discussed offline about just adding a big constant to it
--
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] mumrah commented on pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12946:
URL: https://github.com/apache/kafka/pull/12946#issuecomment-1343113991
Latest build looks good. A few unrelated flaky tests:
```
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.consumer.CooperativeStickyAssignorTest.testLargeAssignmentAndGroupWithNonEqualSubscription() | 1 min 2 sec | 1
Build / JDK 8 and Scala 2.12 / kafka.api.TransactionsExpirationTest.testTransactionAfterProducerIdExpires(String).quorum=kraft | 8.8 sec | 1
Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.integration.SmokeTestDriverIntegrationTest.[1] true
```
--
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] mumrah merged pull request #12946: KAFKA-14427 ZK client support for migrations
Posted by GitBox <gi...@apache.org>.
mumrah merged PR #12946:
URL: https://github.com/apache/kafka/pull/12946
--
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