Skip to content

Conversation

@zhiheng123
Copy link
Contributor

@zhiheng123 zhiheng123 commented May 26, 2024

This addresses the remaining gaps of #4289 in handling ByteBuf retain/release. This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: #791 (comment) .

This PR fixes several problems:

  • ByteString buffer lifecycle in client, follows ByteBufList lifecycle
  • ByteBufList lifecycle, moved to write promise
  • Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises.
  • add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite
    • use these callback actions for proper cleanup
  • extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString
  • properly handle releasing of ByteBufList in the write promise
  • properly handle calling promises that are buffered while authentication is in progress

Descriptions of the changes in this PR:

Fix #xyz

Main Issue: #xyz

BP: #xyz

Motivation

(Explain: why you're making that change, what is the problem you're trying to solve)

Changes

(Describe: what changes you have made)


In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks.


Be sure to do all the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

…pache#4293)

This addresses the remaining gaps of apache#4289 in handling ByteBuf retain/release.
This PR will also address the concern about NioBuffer lifecycle brought up in the review of the original PR review: apache#791 (comment) .

This PR fixes several problems:
* ByteString buffer lifecycle in client, follows ByteBufList lifecycle
* ByteBufList lifecycle, moved to write promise
* Calling of write promises in AuthHandler which buffers messages while authentication is in progress. It was ignoring the promises.

- add 2 callback parameters to writeAndFlush: cleanupActionFailedBeforeWrite and cleanupActionAfterWrite
  - use these callback actions for proper cleanup
- extract a utility class ByteStringUtil for wrapping ByteBufList or ByteBuf as concatenated zero copy ByteString
- properly handle releasing of ByteBufList in the write promise
- properly handle calling promises that are buffered while authentication is in progress
@hezhangjian hezhangjian changed the title fix: reference counting (retain/release) in PerChannelBookieClient (#… [branch 4.16] fix: reference counting (retain/release) in PerChannelBookieClient (#… May 26, 2024
@hezhangjian
Copy link
Member

@lhotari PTAL

@hezhangjian
Copy link
Member

Error: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.12.1:compile (default-compile) on project bookkeeper-server: Compilation failure: Compilation failure:
Error: /home/runner/work/bookkeeper/bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:[1002,29] cannot find symbol
Error: symbol: class BatchedReadEntryCallback
Error: location: class org.apache.bookkeeper.proto.PerChannelBookieClient
Error: /home/runner/work/bookkeeper/bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:[1019,44] cannot find symbol
Error: symbol: class BatchedReadEntryCallback
Error: location: class org.apache.bookkeeper.proto.PerChannelBookieClient
Error: -> [Help 1]

@hezhangjian hezhangjian changed the title [branch 4.16] fix: reference counting (retain/release) in PerChannelBookieClient (#… [branch 4.16] fix: reference counting (retain/release) in PerChannelBookieClient May 27, 2024
@hezhangjian hezhangjian changed the title [branch 4.16] fix: reference counting (retain/release) in PerChannelBookieClient [branch-4.16] fix: reference counting (retain/release) in PerChannelBookieClient May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants