This is an automated email from the ASF dual-hosted git repository. yong pushed a commit to branch branch-4.15 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
commit 3a684190f493323bf5b28c48ce295197c76af11b Author: Qiang Huang <[email protected]> AuthorDate: Sun Nov 27 21:35:40 2022 +0800 [fix][server]fix memory leak when operating ledger metadata (#3662) * [fix][server]fix memory leak when operating ledger metadata (cherry picked from commit 109688c1b460621d636d601580a407b721ca282a) --- .../bookkeeper/meta/CleanupLedgerManager.java | 15 +++- .../bookkeeper/meta/CleanupLedgerManagerTest.java | 81 ++++++++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java index a3ce432961..8c0cb0bf90 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java @@ -26,7 +26,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.locks.ReentrantReadWriteLock; - +import lombok.extern.slf4j.Slf4j; import org.apache.bookkeeper.client.BKException; import org.apache.bookkeeper.client.api.LedgerMetadata; import org.apache.bookkeeper.common.concurrent.FutureUtils; @@ -40,6 +40,7 @@ import org.apache.zookeeper.AsyncCallback; /** * A ledger manager that cleans up resources upon closing. */ +@Slf4j public class CleanupLedgerManager implements LedgerManager { private class CleanupGenericCallback<T> implements GenericCallback<T> { @@ -113,7 +114,17 @@ public class CleanupLedgerManager implements LedgerManager { private void recordPromise(CompletableFuture<?> promise) { futures.add(promise); - promise.thenRun(() -> futures.remove(promise)); + promise.whenComplete((result, exception) -> { + futures.remove(promise); + if (exception != null) { + log.error("Failed on operating ledger metadata: {}", BKException.getExceptionCode(exception)); + } + }); + } + + @VisibleForTesting + int getCurrentFuturePromiseSize() { + return futures.size(); } @Override diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/CleanupLedgerManagerTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/CleanupLedgerManagerTest.java new file mode 100644 index 0000000000..fabbdc7f05 --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/CleanupLedgerManagerTest.java @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.bookkeeper.meta; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.concurrent.CompletableFuture; +import org.apache.bookkeeper.client.api.LedgerMetadata; +import org.apache.bookkeeper.versioning.Version; +import org.apache.bookkeeper.versioning.Versioned; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +/** + * Unit test of {@link CleanupLedgerManager}. + */ +public class CleanupLedgerManagerTest { + + protected LedgerManager ledgerManager = null; + protected CleanupLedgerManager cleanupLedgerManager = null; + + @Before + public void setup() throws Exception { + ledgerManager = mock(LedgerManager.class); + CompletableFuture<Versioned<LedgerMetadata>> future = new CompletableFuture<>(); + future.completeExceptionally(new Exception("LedgerNotExistException")); + when(ledgerManager.createLedgerMetadata(anyLong(), any())).thenReturn(future); + when(ledgerManager.readLedgerMetadata(anyLong())).thenReturn(future); + when(ledgerManager.writeLedgerMetadata(anyLong(), any(), any())).thenReturn( + future); + CompletableFuture<Void> removeFuture = new CompletableFuture<>(); + removeFuture.completeExceptionally(new Exception("LedgerNotExistException")); + when(ledgerManager.removeLedgerMetadata(anyLong(), any())).thenReturn(removeFuture); + cleanupLedgerManager = new CleanupLedgerManager(ledgerManager); + } + + @Test + public void testCreateLedgerMetadataException() throws Exception { + cleanupLedgerManager.createLedgerMetadata(anyLong(), any(LedgerMetadata.class)); + Assert.assertEquals(0, cleanupLedgerManager.getCurrentFuturePromiseSize()); + } + + @Test + public void testReadLedgerMetadataException() throws Exception { + cleanupLedgerManager.readLedgerMetadata(anyLong()); + Assert.assertEquals(0, cleanupLedgerManager.getCurrentFuturePromiseSize()); + } + + @Test + public void testWriteLedgerMetadataException() throws Exception { + cleanupLedgerManager.writeLedgerMetadata(anyLong(), any(LedgerMetadata.class), any(Version.class)); + Assert.assertEquals(0, cleanupLedgerManager.getCurrentFuturePromiseSize()); + } + + @Test + public void testRemoveLedgerMetadataException() throws Exception { + cleanupLedgerManager.removeLedgerMetadata(anyLong(), any(Version.class)); + Assert.assertEquals(0, cleanupLedgerManager.getCurrentFuturePromiseSize()); + } +}
