Skip to content

LedgerOpenOp: Do not call blocking close() in the callback #3513

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.

Sign up for GitHub

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

Jump to bottom
Merged
StevenLuMT merged 2 commits into from
Oct 8, 2022
Merged

LedgerOpenOp: Do not call blocking close() in the callback #3513

StevenLuMT merged 2 commits into from
Oct 8, 2022

Conversation

@dlg99
Copy link
Contributor

@dlg99 dlg99 commented Sep 30, 2022

Descriptions of the changes in this PR:

Motivation

#2794 introduced blocking call in the callback.
I can't tell if it contributes to #3466 but it will lead to slow processing main worker pool under the conditions described in the issue (network retries, Zk session reconnects etc)

Changes

Use async close.

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

@dlg99 dlg99 mentioned this pull request Sep 30, 2022
Closed
@eolivelli
Copy link
Contributor

eolivelli commented Sep 30, 2022

@RaulGracia can you run your test with this change?

eolivelli
eolivelli requested changes Oct 2, 2022
View reviewed changes
eolivelli
eolivelli approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

StevenLuMT
StevenLuMT approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hangc0276 hangc0276 added this to the 4.16.0 milestone Oct 8, 2022
@StevenLuMT StevenLuMT merged commit 2734e8b into apache:master Oct 8, 2022
hangc0276 pushed a commit to streamnative/bookkeeper-achieved that referenced this pull request Oct 28, 2022
@dlg99
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
e94e7db 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
zymap pushed a commit that referenced this pull request Nov 3, 2022
@dlg99 @zymap
LedgerOpenOp: Do not call blocking close() in the callback (#3513)
e903e61 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@dlg99 @hangc0276
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
4b66a0c 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@dlg99 @hangc0276
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
760cacd 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
dlg99 added a commit to datastax/bookkeeper that referenced this pull request Nov 19, 2022
@dlg99
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
7c72992 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
(cherry picked from commit e903e61)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
@dlg99 @nicoloboschi
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
d0efef2 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp

(cherry picked from commit 2734e8b)
(cherry picked from commit 760cacd)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
@dlg99
LedgerOpenOp: Do not call blocking close() in the callback (apache#3513)
8aaf3d9 
* Do not call blocking close() in the callback
     * CR feedback, remove lombok from LedgerOpenOp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hangc0276 hangc0276 hangc0276 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@StevenLuMT StevenLuMT StevenLuMT approved these changes

Assignees

@dlg99 dlg99

Labels

cherry-picked/branch-4.14 cherry-picked/branch-4.15 release/4.14.6 release/4.15.3 type/bug

Projects

None yet

Milestone

4.16.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dlg99 @eolivelli @hangc0276 @StevenLuMT @zymap