You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@bookkeeper.apache.org by "hangc0276 (via GitHub)" <gi...@apache.org> on 2023/04/02 15:37:22 UTC

[GitHub] [bookkeeper] hangc0276 opened a new pull request, #3902: Fix promise lost in ClientSide AuthHandler

hangc0276 opened a new pull request, #3902:
URL: https://github.com/apache/bookkeeper/pull/3902

   ### Motivation
   #3783 changed the AddEntry Request type from `BookieProtocol.AddRequest` to `ByteBuf` or `ByteBufList`, leading to the AddEntry request being added into `waitingForAuth` queue in the client-side AuthHandler
   https://github.com/apache/bookkeeper/blob/0171a408e21a51eb74e18a07df1b0ea71b7638ff/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L787-L791
   
   https://github.com/apache/bookkeeper/blob/0171a408e21a51eb74e18a07df1b0ea71b7638ff/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java#L363-L364
   
   For the requests in the `waitingForAuth`, they will be polled out by `AuthHandshakeCompleteCallback#operationComplete`. 
   https://github.com/apache/bookkeeper/blob/0171a408e21a51eb74e18a07df1b0ea71b7638ff/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java#L426-L436
   
   However, the `waitingForAuth` queue only stores the AddEntry request, and it ignores the related ChannelPromise. When polling requests from the `waitingForAuth` queue in `AuthHandshakeCompleteCallback#operationComplete`, those requests will be written and flushed into the Netty channel without passing any ChannelPromises, and Netty will use `voidPromise` by default.
   
   In a word, the AddEntry request side passed ChannelPromise will be replaced with `voidPromise`, and the original promise will never be complete and the following operations won't execute.
   https://github.com/apache/bookkeeper/blob/0171a408e21a51eb74e18a07df1b0ea71b7638ff/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L1159-L1169
   
   There are two impactions:
   - The `nettyOpLogger ` metric won't update
   - The `addEntryOutstanding` counter metric won't update
   
   ### Changes
   Store the ChannelPromise along with the request in the client-side AuthHandle and pass it to the Netty channel when polling out.
   
   


-- 
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

Re: [PR] Fix promise lost in ClientSide AuthHandler [bookkeeper]

Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.

hangc0276 commented on PR #3902:
URL: https://github.com/apache/bookkeeper/pull/3902#issuecomment-1880355794

   We need to add a unit test for this change, move to the next release


-- 
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