-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19597: Stop the RSM after closing the remote-log reader threads to handle requests gracefully #20342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… to handle requests gracefully During shutdown, when the RSM closes first, then the ongoing requests might throw an error. To handle the ongoing requests gracefully, closing the RSM after closing the remote-log reader thread pools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kamalcph for the PR. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamalcph sorry for the late reviews. I leave two questions. Please take a look
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java
Show resolved
Hide resolved
leaderCopyRLMTasks.clear(); | ||
leaderExpirationRLMTasks.clear(); | ||
followerRLMTasks.clear(); | ||
|
||
Utils.closeQuietly(indexCache, "RemoteIndexCache"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add comments explaining the new close order? Otherwise, it might be unintentionally changed in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed caller -> callee pattern. RemoteIndexCache invokes remoteStorageManagerPlugin to fetch the indexes so changed the shutdown order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed caller -> callee pattern. RemoteIndexCache invokes remoteStorageManagerPlugin to fetch the indexes so changed the shutdown order.
yes, that is good pattern. BTW, it looks like fetchQuotaMetrics
and copyQuotaMetrics
are excluded by the removeMetrics
. Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be a miss to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for confirmation. We will file a minor PR for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open #20394
- Changes: Remove fetchQuotaMetrics and copyQuotaMetrics in RemoteLogManager on close from: #20342 (comment) Reviewers: Kamal Chandraprakash <[email protected]>
During shutdown, when the RSM closes first, then the ongoing requests
might throw an error. To handle the ongoing requests gracefully, closing
the RSM after closing the remote-log reader thread pools.
Reviewers: Satish Duggana [email protected]