You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/04 07:51:21 UTC
[GitHub] [pulsar] Shoothzj opened a new pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Shoothzj opened a new pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134
### Motivation
The memory is leak due to the ManagerLedgerImpl can't be garbage collected.
### Modifications
Before remove the `managedLedgerImpl` from map, call `close()` to cacel the schdule job first.
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
Fix a memory leak bug
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762609262
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1192,6 +1192,7 @@ private long consumedLedgerSize(long ledgerSize, long ledgerEntries, long consum
@Override
public synchronized void asyncTerminate(TerminateCallback callback, Object ctx) {
if (state == State.Fenced) {
+ cancelScheduledTasks();
Review comment:
It seems that similar addition would be needed to asyncClose when the ledger is fenced
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762517304
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1192,6 +1192,7 @@ private long consumedLedgerSize(long ledgerSize, long ledgerEntries, long consum
@Override
public synchronized void asyncTerminate(TerminateCallback callback, Object ctx) {
if (state == State.Fenced) {
+ cancelScheduledTasks();
Review comment:
+1
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762428610
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -352,6 +352,7 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
// Managed ledger is in unusable state. Recreate it.
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
+ l.close();
Review comment:
@lhotari please take a look again
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762422742
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -352,6 +352,7 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
// Managed ledger is in unusable state. Recreate it.
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
+ l.close();
Review comment:
Should it be called asynchronously after the removal from the map? What if close blocks or throws an exception?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985997993
> The memory is leak due to the ManagerLedgerImpl can't be garbage collected.
Can you share more details about the symptoms? Which Pulsar versions are impacted?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762517215
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -353,6 +353,17 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
ledgers.remove(name, existingFuture);
+ l.asyncClose(new CloseCallback() {
Review comment:
Good point. I guess calling asyncClose is sufficient. However there seems to be a bug in the asyncClose method when the ledger is fenced. The method call to cancel the scheduled tasks is missing.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985993884
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762428610
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -352,6 +352,7 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
// Managed ledger is in unusable state. Recreate it.
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
+ l.close();
Review comment:
@lhotari please take a look again :)
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-986093063
Good catch @Shoothzj
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762498781
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -353,6 +353,17 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
ledgers.remove(name, existingFuture);
+ l.asyncClose(new CloseCallback() {
Review comment:
Actually, if the managedLedger in `Fenced` or `Closed` state, call `asyncClose` just
https://github.com/apache/pulsar/blob/ee904063abc8ec90c3c536b28a47a59975175cd4/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1316-L1328
So, should we just call `ManagedLedgerFactoryImpl::close` ?
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762517290
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -353,6 +353,17 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
name, l.getState());
ledgers.remove(name, existingFuture);
+ l.asyncClose(new CloseCallback() {
Review comment:
+1
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj edited a comment on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj edited a comment on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985999481
@lhotari
The `managedledgerImpl` were removed from map, but the schedule tasks didn't stop, So their memory can't be released, leading to a memory leak.
In current branch we maintained, version 2.8 and version 2.9 are impacted.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985999481
>
The `managedledgerImpl` were removed from map, but the schedule tasks didn't stop, So their memory can't be released, leading to a memory leak.
In current branch we maintained, version 2.8 and version 2.9 are impacted.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985997535
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#discussion_r762641409
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1192,6 +1192,7 @@ private long consumedLedgerSize(long ledgerSize, long ledgerEntries, long consum
@Override
public synchronized void asyncTerminate(TerminateCallback callback, Object ctx) {
if (state == State.Fenced) {
+ cancelScheduledTasks();
Review comment:
Yes, yesterday，I found It has been fixed in #12565, it's better to cancel the schedule task when the ledger is fenced. I will test if it fix the memomry leak.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj closed pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj closed pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj removed a comment on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj removed a comment on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-985993884
/pulsarbot run-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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13134: Fix schedule task memory leak in ManagedLedgerFactoryImpl
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13134:
URL: https://github.com/apache/pulsar/pull/13134#issuecomment-986002890
@codelipenghui @hangc0276 @eolivelli PTAL
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org