Add proper exception handler to around eviction_stm's call to replicate #12959
Conversation
- Replicate may throw in the case the gate belonging to the consensus class has closed. The eviction_stm never had proper exception handling to cover this case, this change adds a catch all exception clause around the call to _c->replicate() - Fixes: redpanda-data#12950
- This log is printed when replicate fails, therefore it should be logged at warn (or error) and not info
| _log.warn, | ||
| "Replicating prefix_truncate failed with exception: {}", | ||
| std::current_exception()); | ||
| result = errc::replication_error; |
There was a problem hiding this comment.
just a quick question, is this going to be propagated to Kafka handler ?
There was a problem hiding this comment.
Yeah it eventually will be translated into a kafka::error_code here https://github.com/redpanda-data/redpanda/blob/dev/src/v/kafka/server/replicated_partition.cc#L399 then that is propagated back to the client in the delete_records handler.
There was a problem hiding this comment.
From the linked snippet, wouldn't this return a USE? Curious why that instead of explicitly handling the is_shutdown_exception case with raft::errc::shutting_down which results in a (I think) retriable timeout error?
There was a problem hiding this comment.
consensus::replicate will return errc::shutting_down as an error code not as an exception so that path is already covered. What was happening was gate_closed_exception was being thrown and not caught, you can see that from the bad log lines error in the PR cover.
Now that I think about it , seems like there's maybe a missing gate closed exception handler clause somewhere within the replicate logic.
|
/backport v23.2.x |
The issue is observed as a bad log line error within a delete-records test. A higher level exception handler in the delete records request handling logic caught an exception and logged it at error level making the test fail.
Looking into the cause of the issue, it was observed that within
log_eviction_stm::truncatethere's a call toconsensus::replicatethat does not catch exceptions, which may occur. Note that this is not a severe issue because the code eventually catches the exception and returns the proper error code to the client in its current state.Fixes #12950
Backports Required
Release Notes