KAFKA-19831: Improved error handling in DefaultStateUpdater.

[Nikita-Shupletsov]

• Improved error handling in DefaultStateUpdater to take potential
  failures in Task#maybeCheckpoint into account.
• Improved TaskManager#shutdownStateUpdater to not hang indefinitely if
  the State Updater thread is dead.

Reviewers: Matthias J. Sax [email protected], Lucas Brutschy
[email protected]

[mjsax]

Not sure if I understand this condition. Can you elaborate?

Also: if we fail here, how do we ensure that the test fails? This would be executed on the background StreamsThread not the actual "main" thread running the test.

[Nikita-Shupletsov]

to reproduce the issue, we need to fail during a shutdown.

so what the test does:

• it creates a KS app with 2 threads.
• then kills one thread which causes a rebalance
• then the rebalance hangs here, because we can't flush a task.
• we call streams close
• this call gets unblocked and throws an exception
• that's where we reproduce the issue

in other words: the problem I am trying to fix here reproduces if: we are in rebalance, we have some unhandled tasks in the task updater which can throw an exception, which can kill the thread, and we are trying to close the app.
in this case the stream thread will not do its thing, which in turn will not call TaskManager, which will not call stateUpdater.drainExceptionsAndFailedTasks, which will not remove all failed tasks from the lists. which in turn will cause stateUpdater.tasks to return the failed tasks. and TaskManager will try to delete them.
but because the thread is dead, the delete will never succeed, as the dead thread will never complete the future

[Nikita-Shupletsov]

I will add a comment to the test to explain what exactly we are trying to achieve here

[mjsax]

I understand that we do want to throw here to trigger an error on the state-updater thread. But it's not clear to me, why we would need to block/wait here until we go to "pending shutdown"?

[Nikita-Shupletsov]

what needs to happen to reproduce the issue:
the stream thread is already down(so, we want this condition: https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java#L933 to be false), we also want to have a task in the state updater. and this task needs to throw an exception.

maybe there is an easier way to achieve these conditions, but that's what I was able to come up with

[mjsax]

the stream thread is already down

You mean before the state-updater crashes? Interesting. Not clear to me why though? Can you explain further?

[Nikita-Shupletsov]

before the state-updater crashes

yes. when the stream thread is running it calls StreamThread#checkStateUpdater, which, in turn, calls TaskManager, which calls StateUpdater#drainExceptionsAndFailedTasks.
so it will delete the failed task from the list of tasks(hence StateUpdater#tasks will not return them anymore) and trigger shutdown.
and when we get to the shutdown, we will not have anything in the state updater, so we will not try to remove anything from in the shutdownStateUpdater method, hence we will not get stuck.

it technically can be slightly different: we have some tasks in the state updater, one of them fails and kills the thread, which will be picked up by the stream thread. but if after that we some how add more tasks to the state updater, we will get the same issue.
But I couldn't figure out if it's even possible to add new tasks to the state updater when the thread is shitting down, as it happens only during rebalance(but here I may be wrong)

[mjsax]

This test is somewhat complex. Might be good to add a longer comment to explain what it does.

If I read it correctly, we start up two thread, and each thread get 3 tasks each (and we init 6 stores). We stop on thread, re-trigger the init of 3 stores, and let it crash when we hit "store init no.9".

[mjsax]

It seems we are adding a similar try-catch on multiple places. Should we instead add a new catch block to run() method to have it in a single place (and also avoid that this PR might miss some place where it should get added otherwise, too)?

[Nikita-Shupletsov]

the idea was to handle one task at a time instead of failing all of them together. so if we have 3 tasks, one fails but the other 2 succeed, we will have only one failed task instead of all 3.
similarly to https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java#L363 and https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java#L516

[mjsax]

I see. Hard to read on the PR, as there is no loop here...

But I am not even sure why we would want to fail a task if we cannot checkpoint for this case? We only pause a task, and it seems to be non-fatal if we fail to checkpoint? Wondering if we should swallow the exception entirely and just keep the task w/o the need to move it into error state and hand back to StreamsThreads?

\cc @lucasbru

[lucasbru]

Hmm, the comment of maybeCheckpoint mentions this:

     * @throws TaskMigratedException recoverable error sending changelog records that would cause the task to be removed
     * @throws StreamsException fatal error when flushing the state store, for example sending changelog records failed
     *                          or flushing state store get IO errors; such error should cause the thread to die

That would actually indicate that we have to treat streams exception fatally here. But I haven't yet tracked down where they come from and if they can happen in the state updater. Maybe that comment should be made more precise? But going by the commen, we actually need to fail here.

[Nikita-Shupletsov]

But I haven't yet tracked down where they come from and if they can happen in the state updater

the problem I am trying to address by this PR is pretty much that maybeCheckpoint threw an exception when was called by the state updater. Which caused the state updater thread to die and the stream thread shutdown to hang.
even though it happened during maybeCheckpointTasks not during pauseTasks, but it may as well.

maybeCheckpoint threw an exception because the RockDB failed to flush(I assume due to some issues with the file system)

[Nikita-Shupletsov]

Where does the TaskMigratedException come from. Can it happen, or should be update the javadoc?

I wasn't able to find a way to receive TaskMigratedException frommaybeCheckpoint, as maybeCheckpoint doesn't really try to produce any messages. however, as it's a child of StreamsException it's possible that a custom implementation of a store may throw one(although I can't think of a reason).

Why does the comment indicate that the thread should die on an I/O error?

it's the default behavior: https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java#L453, it's not unique for maybeCheckpoint

[mjsax]

Thanks for reading the JavaDoc comments Lucas. This makes it pretty clear. sending changelog records failed is fatal as it would be data loss, and we cannot proceed. It make sense after the fact -- before we can write the local checkpoint file, we need to flush the producer (missed this point originally).

Why does the comment indicate that the thread should die on an I/O error

For such critical errors, we don't want to have any built-in recovery mechanism (design decision), but let the user opt into auto-recovery via the uncaught exception handler.

As we flush pending producer writes, a TaskMigratedException can originate from this flush. We should still handle this one gracefully, and not let the thread die.

[lucasbru]

Wait, you are saying a StreamsException which is not TaskMigrationException should cause a fatal error, no? Because this is not what the PR is doing now.

[Nikita-Shupletsov]

I think the comment by the thread means the streams thread, not the state updater thread.
because the code I shared above does pretty much that - if we get a StreamsException, we shut down the streams thread

[lucasbru]

Oh! Sorry. My fault. You are right, I misremembered what we are doing with the expections downstream. All good!

[mjsax]

Thanks. One minor thing -- would use a larger timeout of 5 minutes. Otherwise LGTM.

Would like to get a sign off from @lucasbru, too, before merging.

[lucasbru]

LGTM

[mjsax]

Build failed for unknown reason. Rerunning. Did see something similar on another PR. Maybe some issue with the build system? Let's see....

[mjsax]

@Nikita-Shupletsov -- There is failing tests. Can you take a look?

[Nikita-Shupletsov]

@Nikita-Shupletsov -- There is failing tests. Can you take a look?

oops. sorry for that. overlooked.
fixed

[mjsax]

Thanks for the fix!

Merged to trunk and cherry-picked to 4.2 branch. -- We should also backport this to 4.1 (there was too many conflicts for a cherry-pick).

[mjsax]

4.1 back-port: #20958