Skip to content
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

Fix the PendingAddOp is not recycled when LedgerHandler closed #3321

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

zymap
Copy link
Member

@zymap zymap commented Jun 7, 2022


Motivation

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback is executed.

---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.
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.

The change makes sense to me.
I am not sure we buy much advantage by recycling the Op object in this corner case.

I am afraid of possible side-effects that we would find only in the future.

btw +1 from me, good catch !

Copy link
Contributor

@lordcheng10 lordcheng10 left a comment

Choose a reason for hiding this comment

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

nice catch.
I have encountered the problem of too many PendingAddOp objects causing GC in pulsar, this may solve my problem.

@eolivelli eolivelli merged commit 3d01e6e into apache:master Jun 8, 2022
eolivelli pushed a commit that referenced this pull request Jun 8, 2022
---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.

(cherry picked from commit 3d01e6e)
@eolivelli
Copy link
Contributor

I have cherry-picked this change to branch-4.15 and branch-4.14

eolivelli pushed a commit that referenced this pull request Jun 8, 2022
---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.

(cherry picked from commit 3d01e6e)
eolivelli pushed a commit to datastax/bookkeeper that referenced this pull request Jun 8, 2022
…e#3321)

---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.

(cherry picked from commit 3d01e6e)
Shawyeok pushed a commit to Shawyeok/bookkeeper that referenced this pull request Oct 21, 2022
…e#3321)

---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.

(cherry picked from commit 3d01e6e)
hangc0276 pushed a commit that referenced this pull request Nov 14, 2022
…gerHandleAdv (#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to #3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv
hangc0276 pushed a commit that referenced this pull request Nov 14, 2022
…gerHandleAdv (#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to #3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv

(cherry picked from commit 0d2fbe4)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
…gerHandleAdv (apache#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to apache#3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv

(cherry picked from commit 0d2fbe4)
(cherry picked from commit b4d0be2)
yaalsn pushed a commit to yaalsn/bookkeeper that referenced this pull request Jan 30, 2023
…gerHandleAdv (apache#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to apache#3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv
zymap pushed a commit that referenced this pull request Feb 16, 2023
…gerHandleAdv (#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to #3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv

(cherry picked from commit 0d2fbe4)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…e#3321)

---

**Motivation**

When adding an entry into a bookie, the entry data lifecycle
is handled by the bookie client. The data buffer will be
released after receiving a corresponding response from the
bookie server. So the user doesn't care about the entry
buffer releasing.
But when the ledgerHandler is closed, the PendingAddOp is not
recycled which leads to the data buffer never being released.
We should release that after the callback executed.
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…gerHandleAdv (apache#3621)

Descriptions of the changes in this PR:
Apply recycle logic during add entry creation but ledger close to LedgerHandleAdv

### Motivation
This pr is a supplement to apache#3321
If I understand correctly. these recycled logic should be applied to LedgerHandleAdv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants