You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/06/25 08:09:47 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #3361: Fix autoRecover memory leak.
horizonzy opened a new pull request, #3361:
URL: https://github.com/apache/bookkeeper/pull/3361
Descriptions of the changes in this PR:
fixes-#3360
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#issuecomment-1195250983
> @hangc0276 please cherry pick to active branches (4.14, 4.15)
@eolivelli sure.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#discussion_r915683953
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -154,6 +155,30 @@ private void handleMetadata(Versioned<LedgerMetadata> result, Throwable exceptio
}
}
+ /**
+ * CancelWatchLedgerMetadataTask class.
+ */
+ protected class CancelWatchLedgerMetadataTask implements Runnable {
+
+ final long ledgerId;
+
+ CancelWatchLedgerMetadataTask(long ledgerId) {
+ this.ledgerId = ledgerId;
+ }
+
+ @Override
+ public void run() {
+ Set<LedgerMetadataListener> listeners = AbstractZkLedgerManager.this.listeners.get(ledgerId);
Review Comment:
Follow the origin code style. See `ReadLedgerMetadataTask`.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
zymap commented on PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#issuecomment-1175643223
@eolivelli @dlg99 Would you like to help to review this PR? Thanks!
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#issuecomment-1166415339
> We need to close the ledger handle in Auditor#checkAllLedgers() to unregister the listener.
>
> https://github.com/apache/bookkeeper/blob/677ccec3eb84f5be1b3556537871e14eb5e8359c/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java#L1333-L1345
In ProcessLostFragmentsCb, it will close it.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#discussion_r915544927
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java:
##########
@@ -154,6 +155,30 @@ private void handleMetadata(Versioned<LedgerMetadata> result, Throwable exceptio
}
}
+ /**
+ * CancelWatchLedgerMetadataTask class.
+ */
+ protected class CancelWatchLedgerMetadataTask implements Runnable {
+
+ final long ledgerId;
+
+ CancelWatchLedgerMetadataTask(long ledgerId) {
+ this.ledgerId = ledgerId;
+ }
+
+ @Override
+ public void run() {
+ Set<LedgerMetadataListener> listeners = AbstractZkLedgerManager.this.listeners.get(ledgerId);
Review Comment:
this is running in the same thread of "unregisterLedgerMetadataListener"
why do you need a inner class ? can't we simply code this as a regular Java method ?
```
private void cancelLedgerWatchers(long ledgerId) {
....
}
```
and call it from unregisterLedgerMetadataListener ?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#discussion_r907031590
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerTest.java:
##########
@@ -824,9 +824,16 @@ public void testUnregisterLedgerMetadataListener() throws Exception {
ledgerStr, true,
KeeperException.Code.OK.intValue(), serDe.serialize(metadata), stat);
+ mockZkRemoveWatcher();
+
// unregister the listener
ledgerManager.unregisterLedgerMetadataListener(ledgerId, listener);
assertFalse(ledgerManager.listeners.containsKey(ledgerId));
+ assertFalse(watchers.containsKey(ledgerStr));
+ verify(mockZk, times(1)).removeWatches(any(String.class),
Review Comment:
```suggestion
verify(mockZk, times(1)).removeWatches(eq(getLedgerPath(ledgerId)),
```
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#issuecomment-1195247140
@hangc0276 please cherry pick to active branches (4.14, 4.15)
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#discussion_r907052855
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerTest.java:
##########
@@ -824,9 +824,16 @@ public void testUnregisterLedgerMetadataListener() throws Exception {
ledgerStr, true,
KeeperException.Code.OK.intValue(), serDe.serialize(metadata), stat);
+ mockZkRemoveWatcher();
+
// unregister the listener
ledgerManager.unregisterLedgerMetadataListener(ledgerId, listener);
assertFalse(ledgerManager.listeners.containsKey(ledgerId));
+ assertFalse(watchers.containsKey(ledgerStr));
+ verify(mockZk, times(1)).removeWatches(any(String.class),
Review Comment:
good suggestion. 👍
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
zymap commented on PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#issuecomment-1194859229
@eolivelli Could you please take another look?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] liuzhuang2017 commented on a diff in pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
liuzhuang2017 commented on code in PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361#discussion_r928485652
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/AbstractZkLedgerManagerTest.java:
##########
@@ -824,9 +824,16 @@ public void testUnregisterLedgerMetadataListener() throws Exception {
ledgerStr, true,
KeeperException.Code.OK.intValue(), serDe.serialize(metadata), stat);
+ mockZkRemoveWatcher();
+
// unregister the listener
ledgerManager.unregisterLedgerMetadataListener(ledgerId, listener);
assertFalse(ledgerManager.listeners.containsKey(ledgerId));
+ assertFalse(watchers.containsKey(ledgerStr));
+ verify(mockZk, times(1)).removeWatches(any(String.class),
Review Comment:
good idea, 👍
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #3361: Fix autoRecover memory leak.
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #3361:
URL: https://github.com/apache/bookkeeper/pull/3361
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org