[kv] Close KV tablets concurrently during shutdown#3651
Conversation
fa4c785 to
5a10051
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@swuferhong Thank you, looks good overall, left one question, PTAL
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@swuferhong Thanks, LGTM 👍
platinumhamburg
left a comment
platinumhamburg
left a comment
There was a problem hiding this comment.
LGTM overall, just two minor comments.
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@swuferhong Thank you, LGTM
If @platinumhamburg doesn't have any further comments, feel free to merge 👍
platinumhamburg
left a comment
platinumhamburg
left a comment
There was a problem hiding this comment.
LGTM, just two minor issues.
| Thread shutdownThread = | ||
| new ExecutorThreadFactory(getServerName() + "-fatal-error-shutdown") | ||
| .newThread(() -> closeAsync(Result.FAILURE)); | ||
| shutdownThread.start(); |
There was a problem hiding this comment.
isJvmFatalError() does not include OutOfMemoryError, so an OOM—including “unable to create new native thread”—takes this branch.
The one-shot shutdown flag is set before the new thread is created. If newThread() or start() throws, shutdown never starts, and later fatal-error callbacks are ignored because the flag is already set.
Please treat OOM as immediately fatal by using isJvmFatalOrOutOfMemoryError(), or catch thread creation/start failures and terminate the process as a fallback. Please also add a test covering this failure path.
3 participants
Purpose
Linked issue: close #3650
KvManager previously closed KV tablets sequentially, causing shutdown latency to grow linearly with the number of tablets.
Brief change log
Tests
API and Format
Documentation