You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/09/14 04:06:30 UTC
[GitHub] [bookkeeper] michaeljmarshall opened a new pull request, #3486: Check if channel closed before processing read request
michaeljmarshall opened a new pull request, #3486:
URL: https://github.com/apache/bookkeeper/pull/3486
### Motivation
When a client disconnects from a bookie server, there is a chance that there are pending read requests in the `BookieRequestProcessor`'s queue. Since these are read operations cannot succeed, there is no reason to attempt them.
The primary motivation is to prevent unnecessary reads to the disk as well as decrease the memory utilization associated with unnecessary reads. Additionally, reads are currently performed as blocking calls on the `BookieRequestProcessor` processing thread pool, so failing fast can increase the processing speed of the thread pool. See:
https://github.com/apache/bookkeeper/blob/a65e888b25eb966921ff39032e04f367a553d634/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java#L57-L58
Note: we cannot add this logic for writes because those can be "recovered" when the client reconnects.
### Changes
* Only process a read request if the channel is open.
### Alternative Designs
If we are concerned about the cost of reading the volatile variable from the channel, we could possibly speed up the logic by only running the check if the operation has been enqueued for over some threshold, like 100 millis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1250180039
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1246867000
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on a diff in pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#discussion_r973228658
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java:
##########
@@ -249,6 +249,12 @@ protected ReadResponse getReadResponse() {
public void safeRun() {
requestProcessor.getRequestStats().getReadEntrySchedulingDelayStats().registerSuccessfulEvent(
MathUtils.elapsedNanos(enqueueNanos), TimeUnit.NANOSECONDS);
+ if (!channel.isOpen()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Dropping read request for closed channel: {}", channel);
+ }
+ return;
Review Comment:
Yes, great catch! Thank you. I definitely should have caught that since I just fixed the same bug last week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1247470021
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1249847896
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1250782951
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi merged pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
nicoloboschi merged PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#issuecomment-1246247534
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3486: Check if channel closed before processing read request
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3486:
URL: https://github.com/apache/bookkeeper/pull/3486#discussion_r971177453
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java:
##########
@@ -249,6 +249,12 @@ protected ReadResponse getReadResponse() {
public void safeRun() {
requestProcessor.getRequestStats().getReadEntrySchedulingDelayStats().registerSuccessfulEvent(
MathUtils.elapsedNanos(enqueueNanos), TimeUnit.NANOSECONDS);
+ if (!channel.isOpen()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Dropping read request for closed channel: {}", channel);
+ }
+ return;
Review Comment:
should it call `requestProcessor.onReadRequestFinish();` before return?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java:
##########
@@ -61,6 +61,12 @@ protected void processPacket() {
if (LOG.isDebugEnabled()) {
LOG.debug("Received new read request: {}", request);
}
+ if (!channel.isOpen()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Dropping read request for closed channel: {}", channel);
+ }
+ return;
Review Comment:
should it call `requestProcessor.onReadRequestFinish();` before return?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org