KAFKA-14712: Produce correct error msg with correct metadataversion by Owen-CH-Leung · Pull Request #13773 · apache/kafka

Owen-CH-Leung · 2023-05-27T10:57:36Z

Fix the confusing error message

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

Fix the confusing error message

Owen-CH-Leung · 2023-05-29T03:19:35Z

@mumrah can I ask for your review ? Thanks

showuon · 2023-06-15T03:02:11Z

@Owen-CH-Leung , thanks for the patch, could you add tests for this change?

Owen-CH-Leung · 2023-06-15T06:33:56Z

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.

mumrah

Hi @Owen-CH-Leung, thanks for the patch! I've made a few suggestions in the PR.

mumrah · 2023-06-15T14:29:41Z

 public final class ImageWriterOptions {
    public static class Builder {
        private MetadataVersion metadataVersion;
+        private MetadataVersion orgMetadataVersion;


How about "requestedMetadataVersion" ?

Thanks. Refactored to requestedMetadataVersion

mumrah · 2023-06-15T14:34:31Z

+                setOrgMetadataVersion(metadataVersion);
                setRawMetadataVersion(MetadataVersion.MINIMUM_KRAFT_VERSION);
            } else {
                setRawMetadataVersion(metadataVersion);


Let's set both metadata versions here as well so we don't have to deal with a null later

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 ?)

Owen-CH-Leung · 2023-06-16T03:07:40Z

@mumrah @showuon 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

showuon · 2023-06-21T06:58:40Z

@Owen-CH-Leung , please try to rebase to the latest trunk branch, the failed testReplication should be fixed. Thanks.

Owen-CH-Leung · 2023-06-22T03:21:06Z

@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]     org.apache.kafka.common.errors.TimeoutException: Timeout expired after 60000ms while awaiting InitProducerId```

showuon

@Owen-CH-Leung , thanks for the PR. Left a comment to the test.

showuon · 2023-07-17T09:46:51Z

+        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());
+        }


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?

@showuon Thanks! Yes it's a lot cleaner. I've adopted your suggestion.

showuon

LGTM! Just a minor comment.

showuon · 2023-07-17T11:13:31Z

+            Consumer<UnwritableMetadataException> customLossHandler = e -> {
+                assertEquals(formattedMessage, e.getMessage());
+            };


nit: no need curly brackets here.

ahh yes. Removed it

showuon · 2023-07-18T01:55:11Z

@mumrah , do you want to have another look? I'm going to merge this later this week if no other comments. Thanks.

showuon · 2023-07-24T02:35:27Z

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()

…pache#13773) Fix the confusing error message in ImageWriterOptions Reviewers: Luke Chen <showuon@gmail.com>, David Arthur <mumrah@gmail.com>

KAFKA-14712: Produce correct error msg with correct metadataversion

ec68962

Fix the confusing error message

Owen-CH-Leung marked this pull request as ready for review May 27, 2023 13:04

Owen-CH-Leung added 2 commits May 29, 2023 21:33

Keep the original build() logic

b94ee40

Merge branch 'trunk' into KAFKA-14712

b4dd894

add unittest for handleLoss function

59135fc

divijvaidya requested a review from mumrah June 15, 2023 09:09

mumrah reviewed Jun 15, 2023

View reviewed changes

refactor variable names

9608077

Owen-CH-Leung requested a review from mumrah June 16, 2023 03:00

Merge branch 'trunk' into KAFKA-14712

f317c63

Merge branch 'apache:trunk' into KAFKA-14712

eb9e98f

showuon reviewed Jul 17, 2023

View reviewed changes

refactor testHandleLoss

094e53e

Owen-CH-Leung requested a review from showuon July 17, 2023 11:04

showuon approved these changes Jul 17, 2023

View reviewed changes

Removed curly brackets

864cab1

Owen-CH-Leung mentioned this pull request Jul 21, 2023

KAFKA-15194-Prepend-Offset-as-Filename #14057

Merged

3 tasks

Merge branch 'apache:trunk' into KAFKA-14712

6a04050

showuon merged commit 4981fa9 into apache:trunk Jul 24, 2023

Uh oh!

Conversation

Owen-CH-Leung commented May 27, 2023

Committer Checklist (excluded from commit message)

Uh oh!

Owen-CH-Leung commented May 29, 2023

Uh oh!

showuon commented Jun 15, 2023

Uh oh!

Owen-CH-Leung commented Jun 15, 2023

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

mumrah Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Owen-CH-Leung Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

mumrah Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Owen-CH-Leung Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Owen-CH-Leung commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Jun 21, 2023

Uh oh!

Owen-CH-Leung commented Jun 22, 2023

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

showuon Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Owen-CH-Leung Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

showuon Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Owen-CH-Leung Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

showuon commented Jul 18, 2023

Uh oh!

showuon commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Owen-CH-Leung commented Jun 16, 2023 •

edited

Loading

Owen-CH-Leung Jul 17, 2023 •

edited

Loading