You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "Owen-CH-Leung (via GitHub)" <gi...@apache.org> on 2023/05/27 10:57:38 UTC
[GitHub] [kafka] Owen-CH-Leung opened a new pull request, #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Owen-CH-Leung opened a new pull request, #13773:
URL: https://github.com/apache/kafka/pull/13773
Fix the confusing error message
### Committer Checklist (excluded from commit message)
- [x] Verify design and implementation
- [x] Verify test coverage and CI build status
- [x] 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] Owen-CH-Leung commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1265177897
##########
metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java:
##########
@@ -44,9 +48,33 @@ public void testSetMetadataVersion() {
setMetadataVersion(version);
if (i < MetadataVersion.MINIMUM_BOOTSTRAP_VERSION.ordinal()) {
assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION, options.metadataVersion());
+ assertEquals(version, options.requestedMetadataVersion());
} else {
assertEquals(version, options.metadataVersion());
}
}
}
+
+ @Test
+ public void testHandleLoss() {
+ PrintStream originalOut = System.out;
+ String expectedMessage = "stuff";
+ Consumer<UnwritableMetadataException> customLossHandler = e -> System.out.println(e.getMessage());
+
+ for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
+ i < MetadataVersion.VERSIONS.length;
+ i++) {
+ ByteArrayOutputStream outContent = new ByteArrayOutputStream();
+ MetadataVersion version = MetadataVersion.VERSIONS[i];
+ ImageWriterOptions options = new ImageWriterOptions.Builder()
+ .setMetadataVersion(version)
+ .setLossHandler(customLossHandler)
+ .build();
+ System.setOut(new PrintStream(outContent));
+ options.handleLoss(expectedMessage);
+ System.setOut(originalOut);
+ String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
+ assertEquals(formattedMessage, outContent.toString().trim());
+ }
Review Comment:
@showuon Thanks! Yes it's a lot cleaner. I've adopted your suggestion.
--
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] Owen-CH-Leung commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1265177897
##########
metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java:
##########
@@ -44,9 +48,33 @@ public void testSetMetadataVersion() {
setMetadataVersion(version);
if (i < MetadataVersion.MINIMUM_BOOTSTRAP_VERSION.ordinal()) {
assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION, options.metadataVersion());
+ assertEquals(version, options.requestedMetadataVersion());
} else {
assertEquals(version, options.metadataVersion());
}
}
}
+
+ @Test
+ public void testHandleLoss() {
+ PrintStream originalOut = System.out;
+ String expectedMessage = "stuff";
+ Consumer<UnwritableMetadataException> customLossHandler = e -> System.out.println(e.getMessage());
+
+ for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
+ i < MetadataVersion.VERSIONS.length;
+ i++) {
+ ByteArrayOutputStream outContent = new ByteArrayOutputStream();
+ MetadataVersion version = MetadataVersion.VERSIONS[i];
+ ImageWriterOptions options = new ImageWriterOptions.Builder()
+ .setMetadataVersion(version)
+ .setLossHandler(customLossHandler)
+ .build();
+ System.setOut(new PrintStream(outContent));
+ options.handleLoss(expectedMessage);
+ System.setOut(originalOut);
+ String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
+ assertEquals(formattedMessage, outContent.toString().trim());
+ }
Review Comment:
@showuon Thanks! Yes it's a lot cleaner. I've adopted your suggestion =D
--
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] Owen-CH-Leung commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1231247557
##########
metadata/src/main/java/org/apache/kafka/image/writer/ImageWriterOptions.java:
##########
@@ -29,6 +29,7 @@
public final class ImageWriterOptions {
public static class Builder {
private MetadataVersion metadataVersion;
+ private MetadataVersion orgMetadataVersion;
Review Comment:
Thanks. Refactored to `requestedMetadataVersion`
--
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] Owen-CH-Leung commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1231250401
##########
metadata/src/main/java/org/apache/kafka/image/writer/ImageWriterOptions.java:
##########
@@ -45,6 +46,7 @@ public Builder setMetadataVersion(MetadataVersion metadataVersion) {
if (metadataVersion.isLessThan(MetadataVersion.MINIMUM_BOOTSTRAP_VERSION)) {
// When writing an image, all versions less than 3.3-IV0 are treated as 3.0-IV1.
// This is because those versions don't support FeatureLevelRecord.
+ setOrgMetadataVersion(metadataVersion);
setRawMetadataVersion(MetadataVersion.MINIMUM_KRAFT_VERSION);
} else {
setRawMetadataVersion(metadataVersion);
Review Comment:
Sure. I've set both fields in this function call. Note that I've also modified the `ImageDowngradeTest` to use the `setMetadataVersion` instead of `setRawMetadataVersion` (Maybe we should change `setRawMetadataVersion` to private` also ?)
--
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] showuon commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1265199695
##########
metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java:
##########
@@ -44,9 +46,30 @@ public void testSetMetadataVersion() {
setMetadataVersion(version);
if (i < MetadataVersion.MINIMUM_BOOTSTRAP_VERSION.ordinal()) {
assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION, options.metadataVersion());
+ assertEquals(version, options.requestedMetadataVersion());
} else {
assertEquals(version, options.metadataVersion());
}
}
}
+
+ @Test
+ public void testHandleLoss() {
+ String expectedMessage = "stuff";
+
+ for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
+ i < MetadataVersion.VERSIONS.length;
+ i++) {
+ MetadataVersion version = MetadataVersion.VERSIONS[i];
+ String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
+ Consumer<UnwritableMetadataException> customLossHandler = e -> {
+ assertEquals(formattedMessage, e.getMessage());
+ };
Review Comment:
nit: no need curly brackets here.
--
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] showuon commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1592269594
@Owen-CH-Leung , thanks for the patch, could you add tests for this change?
--
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] Owen-CH-Leung commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1592447477
> b4dd894
@showuon Thanks a lot for your feedback. I've added tests to test the `handleLoss` function. The issue as described in KAFKA-14712 should go away 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] showuon commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1647109827
Failed tests are unrelated:
```
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 17 and Scala 2.13 / kafka.server.KafkaServerKRaftRegistrationTest.[1] Type=ZK, Name=testRegisterZkBrokerInKraft, MetadataVersion=3.4-IV0, Security=PLAINTEXT
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testSyncTopicConfigs()
Build / JDK 20 and Scala 2.13 / kafka.log.remote.RemoteIndexCacheTest.testCacheEntryExpiry()
Build / JDK 20 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
```
--
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] Owen-CH-Leung commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1265218200
##########
metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java:
##########
@@ -44,9 +46,30 @@ public void testSetMetadataVersion() {
setMetadataVersion(version);
if (i < MetadataVersion.MINIMUM_BOOTSTRAP_VERSION.ordinal()) {
assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION, options.metadataVersion());
+ assertEquals(version, options.requestedMetadataVersion());
} else {
assertEquals(version, options.metadataVersion());
}
}
}
+
+ @Test
+ public void testHandleLoss() {
+ String expectedMessage = "stuff";
+
+ for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
+ i < MetadataVersion.VERSIONS.length;
+ i++) {
+ MetadataVersion version = MetadataVersion.VERSIONS[i];
+ String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
+ Consumer<UnwritableMetadataException> customLossHandler = e -> {
+ assertEquals(formattedMessage, e.getMessage());
+ };
Review Comment:
ahh yes. Removed 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] showuon commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1600287165
@Owen-CH-Leung , please try to rebase to the latest trunk branch, the failed `testReplication` should be fixed. 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Owen-CH-Leung commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1566445925
@mumrah can I ask for your review ? 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1231113907
##########
metadata/src/main/java/org/apache/kafka/image/writer/ImageWriterOptions.java:
##########
@@ -45,6 +46,7 @@ public Builder setMetadataVersion(MetadataVersion metadataVersion) {
if (metadataVersion.isLessThan(MetadataVersion.MINIMUM_BOOTSTRAP_VERSION)) {
// When writing an image, all versions less than 3.3-IV0 are treated as 3.0-IV1.
// This is because those versions don't support FeatureLevelRecord.
+ setOrgMetadataVersion(metadataVersion);
setRawMetadataVersion(MetadataVersion.MINIMUM_KRAFT_VERSION);
} else {
setRawMetadataVersion(metadataVersion);
Review Comment:
Let's set both metadata versions here as well so we don't have to deal with a `null` later
##########
metadata/src/main/java/org/apache/kafka/image/writer/ImageWriterOptions.java:
##########
@@ -29,6 +29,7 @@
public final class ImageWriterOptions {
public static class Builder {
private MetadataVersion metadataVersion;
+ private MetadataVersion orgMetadataVersion;
Review Comment:
How about "requestedMetadataVersion" ?
--
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] showuon commented on a diff in pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13773:
URL: https://github.com/apache/kafka/pull/13773#discussion_r1265116940
##########
metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java:
##########
@@ -44,9 +48,33 @@ public void testSetMetadataVersion() {
setMetadataVersion(version);
if (i < MetadataVersion.MINIMUM_BOOTSTRAP_VERSION.ordinal()) {
assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION, options.metadataVersion());
+ assertEquals(version, options.requestedMetadataVersion());
} else {
assertEquals(version, options.metadataVersion());
}
}
}
+
+ @Test
+ public void testHandleLoss() {
+ PrintStream originalOut = System.out;
+ String expectedMessage = "stuff";
+ Consumer<UnwritableMetadataException> customLossHandler = e -> System.out.println(e.getMessage());
+
+ for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
+ i < MetadataVersion.VERSIONS.length;
+ i++) {
+ ByteArrayOutputStream outContent = new ByteArrayOutputStream();
+ MetadataVersion version = MetadataVersion.VERSIONS[i];
+ ImageWriterOptions options = new ImageWriterOptions.Builder()
+ .setMetadataVersion(version)
+ .setLossHandler(customLossHandler)
+ .build();
+ System.setOut(new PrintStream(outContent));
+ options.handleLoss(expectedMessage);
+ System.setOut(originalOut);
+ String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
+ assertEquals(formattedMessage, outContent.toString().trim());
+ }
Review Comment:
I think we can simplify this test as this without relying System.out:
```
for (int i = MetadataVersion.MINIMUM_KRAFT_VERSION.ordinal();
i < MetadataVersion.VERSIONS.length;
i++) {
MetadataVersion version = MetadataVersion.VERSIONS[i];
String formattedMessage = String.format("Metadata has been lost because the following could not be represented in metadata version %s: %s", version, expectedMessage);
Consumer<UnwritableMetadataException> customLossHandler = e -> {
assertEquals(formattedMessage, e.getMessage());
};
ImageWriterOptions options = new ImageWriterOptions.Builder()
.setMetadataVersion(version)
.setLossHandler(customLossHandler)
.build();
options.handleLoss(expectedMessage);
}
```
WDYT?
--
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] showuon merged pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #13773:
URL: https://github.com/apache/kafka/pull/13773
--
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] Owen-CH-Leung commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1601960188
@showuon Thanks. I just rebase to the latest trunk branch. But I saw there're still tests failed for `core:integrationTest`
```[2023-06-21T15:53:46.873Z] Gradle Test Run :core:integrationTest > Gradle Test Executor 186 > TransactionsTest > testBumpTransactionalEpoch(String) > testBumpTransactionalEpoch(String).quorum=kraft FAILED
[2023-06-21T15:53:46.873Z] org.apache.kafka.common.errors.TimeoutException: Timeout expired after 60000ms while awaiting InitProducerId```
--
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] Owen-CH-Leung commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "Owen-CH-Leung (via GitHub)" <gi...@apache.org>.
Owen-CH-Leung commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1594025469
I saw there're a few unit test failed in the CI job (e.g. `MirrorConnectorsIntegrationExactlyOnceTest.testReplication()`, but I don't think it's related to my changes ? Can someone take a look and give me some hints as in how I can get a green CI ? Thanks a lot
--
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] showuon commented on pull request #13773: KAFKA-14712: Produce correct error msg with correct metadataversion
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13773:
URL: https://github.com/apache/kafka/pull/13773#issuecomment-1639160606
@mumrah , do you want to have another look? I'm going to merge this later this week if no other comments. 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org