Skip to content

MINOR: Improved error handling in ZK migration#13372

Merged
cmccabe merged 15 commits into
apache:trunkfrom
mumrah:minor-better-zk-migration-error-handling
Mar 16, 2023
Merged

MINOR: Improved error handling in ZK migration#13372
cmccabe merged 15 commits into
apache:trunkfrom
mumrah:minor-better-zk-migration-error-handling

Conversation

@mumrah

@mumrah mumrah commented Mar 9, 2023

Copy link
Copy Markdown
Member

This patch fixes many small issues to improve error handling and logging during the ZK migration. A test was added to simulate a ZK session expiration to ensure the correctness of the migration driver.

With this change, ZK errors thrown during the migration will not hit the fault handler registered with with KRaftMigrationDriver, but they will be logged.

@mumrah mumrah assigned cmccabe and mumrah and unassigned cmccabe Mar 13, 2023
@mumrah
mumrah requested a review from cmccabe March 13, 2023 19:26
Comment thread core/src/main/scala/kafka/zk/ZkMigrationClient.scala Outdated
Comment thread core/src/main/scala/kafka/zk/ZkMigrationClient.scala Outdated
Comment thread core/src/main/scala/kafka/zk/ZkMigrationClient.scala Outdated
}

// First check the brokers registered in ZK
Set<Integer> zkBrokerRegistrations = new HashSet<>(zkMigrationClient.readBrokerIds());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to copy the Set<Integer> we get back from zkMigrationClient.readBrokerIds? Seems like it should be fine to use directly...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yea this is needed because the Set returned by the client was an immutable Scala thing originally. I'll change the client to return a mutable set

@mumrah mumrah added the kraft label Mar 14, 2023
@cmccabe

cmccabe commented Mar 15, 2023

Copy link
Copy Markdown
Contributor

@mumrah : KRaftMigrationDriverTest.testMigrationWithClientException is failing for me with this PR. Can you take a look?

Gradle Test Run :metadata:test > Gradle Test Executor 3 > KRaftMigrationDriverTest > testMigrationWithClientException(boolean) > org.apache.kafka.metadata.migration.KRaftMigrationDriverTest.testMigrationWithClientException(boolean)[1] FAILED
    org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at app//org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:180)
        at app//org.apache.kafka.metadata.migration.KRaftMigrationDriverTest.testMigrationWithClientException(KRaftMigrationDriverTest.java:358)```

(Also Jenkins did something silly again, but hopefully the next build will work.)

@mumrah mumrah assigned cmccabe and mumrah and unassigned mumrah Mar 16, 2023
@mumrah

mumrah commented Mar 16, 2023

Copy link
Copy Markdown
Member Author

@cmccabe the latest test run fixed the unit test. The other failures will be addressed by #13407

@cmccabe
cmccabe merged commit 5dcdf71 into apache:trunk Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants