KAFKA-16563: retry pollEvent in KRaftMigrationDriver for retriable errors by showuon · Pull Request #15732 · apache/kafka

showuon · 2024-04-16T12:04:32Z

When running ZK migrating to KRaft process, we encountered an issue that the migrating is hanging and the ZkMigrationState cannot move to MIGRATION state. And it is because the pollEvent didn't retry with the retriable MigrationClientException (ZK client retriable errors) while it should. This PR fixes it and add test. And because of this, the poll event will not poll anymore, which causes the KRaftMigrationDriver hanging.

We could consider to let the leader node do the znode creation only to avoid this conflict issue. But that will be another improvement.

Committer Checklist (excluded from commit message)

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

…rors

showuon · 2024-04-16T12:05:10Z

@cmccabe @mumrah , call for review. Thanks.

soarez · 2024-04-16T14:28:02Z

metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

+        // Use no-op handler by default because the handleException will be overridden if needed
+        private Consumer<Throwable> retryHandler = NO_OP_HANDLER;
+
+        public void retryHandler(Consumer<Throwable> retryHandler) {
+            this.retryHandler = retryHandler;
+        }


Did you consider simply defining an empty public void retryHandler(Throwable thrown) that PollEvent can override?

+1 to this style!

Also, should we call wakeup (run next poll ASAP) rather that scheduleDeferred if the exception is retryable?

Did you consider simply defining an empty public void retryHandler(Throwable thrown) that PollEvent can override?

Nice suggestion! Updated!

Also, should we call wakeup (run next poll ASAP) rather that scheduleDeferred if the exception is retryable?

Thanks for the suggestion. I think that's not appropriate because if the retriable error needs some time to be fixed (ex: the ZK connection issue), the pollEvent will be invoked a lot of times (and keep retrying) in a short period of time.

chia7712 · 2024-04-17T06:46:18Z

metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

                KRaftMigrationDriver.this.faultHandler.handleFault("Encountered ZooKeeper authentication in " + this, e);
            } else if (e instanceof MigrationClientException) {
                log.info(String.format("Encountered ZooKeeper error during event %s. Will retry.", this), e.getCause());
+                retryHandler();


I feel the retry is existent except for UNINITIALIZED since UNINITIALIZED is not running by another event. For other event type, PollEvent will put (do-something event + one deferred PollEvent) to the queue. It means the deferred PollEvent is the "retry".

My question is "why we did not handle UNINITIALIZED by another event"? If we move recoverMigrationStateFromZK to another event, we don't need to add extra retryHandler.

Also, the solution offered by this PR has a side effect that we will put 2 PollEvent if the exception MigrationClientException happens in other migrationState

My question is "why we did not handle UNINITIALIZED by another event"? If we move recoverMigrationStateFromZK to another event, we don't need to add extra retryHandler.

That's a good quesiton, @chia7712 ! Let me think about it.

Also, the solution offered by this PR has a side effect that we will put 2 PollEvent if the exception MigrationClientException happens in other migrationState

No, as you said above, the MigrationClientException retryHandler won't be triggered in other migrationState because they will be handled in other event handler, which is not related to pollEvent. And because the default retryHandler is no-op, there will be no retry for other migrationStates. As long as pollEvent is keep polling, they can be retried later.

No, as you said above, the MigrationClientException retryHandler won't be triggered in other migrationState because they will be handled in other event handler, which is not related to pollEvent. And because the default retryHandler is no-op, there will be no retry for other migrationStates. As long as pollEvent is keep polling, they can be retried later.

you are right :)

@chia7712 , I take your suggestion to add RecoverMigrationStateFromZKEvent so that we don't need to worry about retry anymore. I was checking if this change will cause any side effect, and here is my finding:

recoverMigrationStateFromZK is expected to run before the driver starts the state machine.

In the recoverMigrationStateFromZK, we'll do these things:
a. create a ZNode for migration and initial migration state
b. install this class as a metadata publisher
c. transition to INACTIVE state

If this recoverMigrationStateFromZK is keep failing, the log will keep outputting errors and keep retrying. Once it succeeds, the metadata publisher will be installed and the onControllerChange and onMetadataUpdate will be triggered to start the process. That means, if we change recoverMigrationStateFromZK into an event, it won't affect anything because what we need to do at this state is just waiting for the (a)(b)(c) operation completes.

So, I'm +1 with this suggestion. Thank you.

chia7712

@showuon thanks for sharing the survey. Leave some comments below.

However, I'm still not sure why #12998 did not use event to recover migration state from zk. @mumrah @cmccabe Could you take a look and share your views to me? thanks!

metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

chia7712 · 2024-04-18T14:01:32Z

metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

            switch (migrationState) {
                case UNINITIALIZED:
-                    recoverMigrationStateFromZK();
+                    eventQueue.append(new RecoverMigrationStateFromZKEvent());


Should we use prepend to make sure this event is executed ASAP

No need. Like I said in this comment, in the UNINITIALIZED state, the only event we will receive is the pollEvent. We'll receive additionalonControllerChange (KRaftLeaderEvent) and onMetadataUpdate (MetadataChangeEvent) after completing RecoverMigrationStateFromZKEvent. So, we don't have to worry about the order at this moment.

Are we allowing a race between the RecoverMigrationStateFromZKEvent and the next PollEvent scheduled after the switch? Maybe this could be more straightforward if we only schedule the next poll once RecoverMigrationStateFromZKEvent finishes, either normally or exceptionally? WDYT?

On second thought, I don't think my question makes sense. The following PollEvent can only after RecoverMigrationStateFromZKEvent finishes.

showuon · 2024-04-19T08:24:58Z

metadata/src/test/java/org/apache/kafka/metadata/migration/KRaftMigrationDriverTest.java

+    // Wait until the driver has recovered MigrationState From ZK. This is to simulate the driver needs to be installed as the metadata publisher
+    // so that it can receive onControllerChange (KRaftLeaderEvent) and onMetadataUpdate (MetadataChangeEvent) events.
+    private void startAndWaitForRecoveringMigrationStateFromZK(KRaftMigrationDriver driver) throws InterruptedException {
+        driver.start();
+        TestUtils.waitForCondition(() -> driver.migrationState().get(1, TimeUnit.MINUTES).equals(MigrationDriverState.INACTIVE),
+                "Waiting for KRaftMigrationDriver to enter INACTIVE state");


This is necessary now because in the test suite, we might invoke onControllerChange to append KRaftLeaderEvent before the RecoverMigrationStateFromZKEvent is appended. This won't happen in practice because the driver needs to wait until RecoverMigrationStateFromZKEvent completed to register metadata publisher to receive KRaftLeaderEvent and MetadataChangeEvent.

showuon · 2024-04-19T08:25:31Z

metadata/src/test/java/org/apache/kafka/metadata/migration/KRaftMigrationDriverTest.java

            TestUtils.waitForCondition(() -> driver.migrationState().get(1, TimeUnit.MINUTES).equals(MigrationDriverState.DUAL_WRITE),
-                    "Waiting for KRaftMigrationDriver to enter ZK_MIGRATION state");
+                    "Waiting for KRaftMigrationDriver to enter DUAL_WRITE state");


showuon · 2024-04-22T03:13:07Z

@akhileshchg @mumrah @cmccabe , could you take a look when available. Thanks.

showuon · 2024-04-25T09:23:12Z

@akhileshchg @mumrah @cmccabe , we need your comment on this. Thanks.

akhileshchg

LGTM

akhileshchg · 2024-04-25T23:52:32Z

metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

+        @Override
+        public void run() throws Exception {
+            if (checkDriverState(MigrationDriverState.UNINITIALIZED, this)) {
+                applyMigrationOperation("Recovering migration state from ZK", zkMigrationClient::getOrCreateMigrationRecoveryState);


For my understanding, was this the line where uncaught exception is thrown? Can we handle the exception more gracefully and log and error?

Yes, this is where the uncaught exception thrown. The exception will be handled by its parent
MigrationEvent#handleException, and we'll log error there, and even call the faultHandler.handleFault to handle fatal errors.

kafka/metadata/src/main/java/org/apache/kafka/metadata/migration/KRaftMigrationDriver.java

Lines 396 to 404 in 994077e

public void handleException(Throwable e) {

if (e instanceof MigrationClientAuthException) {

KRaftMigrationDriver.this.faultHandler.handleFault("Encountered ZooKeeper authentication in " + this, e);

} else if (e instanceof MigrationClientException) {

log.info(String.format("Encountered ZooKeeper error during event %s. Will retry.", this), e.getCause());

} else if (e instanceof RejectedExecutionException) {

log.debug("Not processing {} because the event queue is closed.", this);

} else {

KRaftMigrationDriver.this.faultHandler.handleFault("Unhandled error in " + this, e);

Thanks.

showuon · 2024-04-29T08:31:31Z

@akhileshchg , thanks for the review and the approval. But just curious:

However, I'm still not sure why #12998 did not use event to recover migration state from zk.

Do we have any special reason for it?

showuon · 2024-04-29T09:00:46Z

@soarez @chia7712 , since the original author @akhileshchg had reviewed and approved, do you have any other comments?

chia7712

LGTM

soarez

LGTM thanks @showuon

showuon · 2024-04-29T09:45:10Z

Thanks all for the review!

…rors (#15732) When running ZK migrating to KRaft process, we encountered an issue that the migrating is hanging and the ZkMigrationState cannot move to MIGRATION state. And it is because the pollEvent didn't retry with the retriable MigrationClientException (ZK client retriable errors) while it should. This PR fixes it and add test. And because of this, the poll event will not poll anymore, which causes the KRaftMigrationDriver hanging. Reviewers: Luke Chen <showuon@gmail.com>, Igor Soarez<soarez@apple.com>, Akhilesh C <akhileshchg@users.noreply.github.com>

…rors (apache#15732) When running ZK migrating to KRaft process, we encountered an issue that the migrating is hanging and the ZkMigrationState cannot move to MIGRATION state. And it is because the pollEvent didn't retry with the retriable MigrationClientException (ZK client retriable errors) while it should. This PR fixes it and add test. And because of this, the poll event will not poll anymore, which causes the KRaftMigrationDriver hanging. Reviewers: Luke Chen <showuon@gmail.com>, Igor Soarez<soarez@apple.com>, Akhilesh C <akhileshchg@users.noreply.github.com>

KAFKA-16563: retry pollEvent in KRaftMigrationDriver for retriable er…

b5b796f

…rors

soarez reviewed Apr 16, 2024

View reviewed changes

KAFKA-16563: address comments to refactor code

88adc82

chia7712 reviewed Apr 17, 2024

View reviewed changes

KAFKA-16563: change to use event for recoverMigrationStateFromZK

617bc1c

chia7712 reviewed Apr 18, 2024

View reviewed changes

KAFKA-16563: add checkDriverState and fix tests

e800634

showuon force-pushed the KAFKA-16563 branch from 7098690 to e800634 Compare April 19, 2024 07:57

showuon commented Apr 19, 2024

View reviewed changes

akhileshchg approved these changes Apr 26, 2024

View reviewed changes

chia7712 approved these changes Apr 29, 2024

View reviewed changes

soarez approved these changes Apr 29, 2024

View reviewed changes

showuon merged commit ec151c8 into apache:trunk Apr 29, 2024

	public void handleException(Throwable e) {
	if (e instanceof MigrationClientAuthException) {
	KRaftMigrationDriver.this.faultHandler.handleFault("Encountered ZooKeeper authentication in " + this, e);
	} else if (e instanceof MigrationClientException) {
	log.info(String.format("Encountered ZooKeeper error during event %s. Will retry.", this), e.getCause());
	} else if (e instanceof RejectedExecutionException) {
	log.debug("Not processing {} because the event queue is closed.", this);
	} else {
	KRaftMigrationDriver.this.faultHandler.handleFault("Unhandled error in " + this, e);

Conversation

showuon commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

showuon commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon Apr 17, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Apr 22, 2024

Uh oh!

showuon commented Apr 25, 2024

Uh oh!

akhileshchg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Apr 29, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

soarez left a comment

Choose a reason for hiding this comment

Uh oh!

showuon commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

showuon commented Apr 16, 2024 •

edited

Loading

showuon Apr 17, 2024 •

edited

Loading

showuon commented Apr 29, 2024 •

edited

Loading