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 9701a7a432c61381f8ae9c8f7384b67200c87a10 Author: Hang Chen <[email protected]> AuthorDate: Mon Jun 26 09:09:21 2023 +0800 Fix trigger GC not work (#3998) * Fix trigger gc not work (cherry picked from commit 147b5bf846189584d6a9b4e20ac9a73e6e617255) --- .../org/apache/bookkeeper/http/HttpServer.java | 1 + .../bookkeeper/bookie/GarbageCollectorThread.java | 5 +- .../bookie/InterleavedLedgerStorage.java | 2 +- .../apache/bookkeeper/bookie/LedgerStorage.java | 2 +- .../bookkeeper/bookie/SortedLedgerStorage.java | 2 +- .../bookie/storage/ldb/DbLedgerStorage.java | 2 +- .../ldb/SingleDirectoryDbLedgerStorage.java | 2 +- .../server/http/service/TriggerGCService.java | 72 +++++----- .../bookkeeper/bookie/MockLedgerStorage.java | 2 +- .../server/http/service/TriggerGCServiceTest.java | 145 +++++++++++++++++++++ 10 files changed, 195 insertions(+), 40 deletions(-) diff --git a/bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServer.java b/bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServer.java index e614b1e712..7b72868df2 100644 --- a/bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServer.java +++ b/bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServer.java @@ -36,6 +36,7 @@ public interface HttpServer { BAD_REQUEST(400), FORBIDDEN(403), NOT_FOUND(404), + METHOD_NOT_ALLOWED(405), INTERNAL_ERROR(500), SERVICE_UNAVAILABLE(503); diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java index 919853d5d2..bbf8ebdf4d 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java @@ -296,12 +296,11 @@ public class GarbageCollectorThread extends SafeRunnable { } } - public void enableForceGC(Boolean forceMajor, Boolean forceMinor) { + public void enableForceGC(boolean forceMajor, boolean forceMinor) { if (forceGarbageCollection.compareAndSet(false, true)) { LOG.info("Forced garbage collection triggered by thread: {}, forceMajor: {}, forceMinor: {}", Thread.currentThread().getName(), forceMajor, forceMinor); - triggerGC(true, forceMajor == null ? suspendMajorCompaction.get() : !forceMajor, - forceMinor == null ? suspendMinorCompaction.get() : !forceMinor); + triggerGC(true, !forceMajor, !forceMinor); } } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java index a1dc14b2f6..95ecb0fca2 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java @@ -262,7 +262,7 @@ public class InterleavedLedgerStorage implements CompactableLedgerStorage, Entry } @Override - public void forceGC(Boolean forceMajor, Boolean forceMinor) { + public void forceGC(boolean forceMajor, boolean forceMinor) { gcThread.enableForceGC(forceMajor, forceMinor); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java index 8f8f19a9b4..cfe65d509f 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java @@ -233,7 +233,7 @@ public interface LedgerStorage { /** * Force trigger Garbage Collection with forceMajor or forceMinor parameter. */ - default void forceGC(Boolean forceMajor, Boolean forceMinor) { + default void forceGC(boolean forceMajor, boolean forceMinor) { return; } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java index 428a126271..b4dfaf6434 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java @@ -371,7 +371,7 @@ public class SortedLedgerStorage } @Override - public void forceGC(Boolean forceMajor, Boolean forceMinor) { + public void forceGC(boolean forceMajor, boolean forceMinor) { interleavedLedgerStorage.forceGC(forceMajor, forceMinor); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java index 7d33e5eb5e..1b3dd873b7 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java @@ -404,7 +404,7 @@ public class DbLedgerStorage implements LedgerStorage { } @Override - public void forceGC(Boolean forceMajor, Boolean forceMinor) { + public void forceGC(boolean forceMajor, boolean forceMinor) { ledgerStorageList.stream().forEach(s -> s.forceGC(forceMajor, forceMinor)); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java index d1f186d35a..e132953342 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java @@ -252,7 +252,7 @@ public class SingleDirectoryDbLedgerStorage implements CompactableLedgerStorage } @Override - public void forceGC(Boolean forceMajor, Boolean forceMinor) { + public void forceGC(boolean forceMajor, boolean forceMinor) { gcThread.enableForceGC(forceMajor, forceMinor); } diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java index 9f01c5c1b4..c1e25f5554 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java @@ -22,6 +22,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.util.HashMap; import java.util.Map; +import org.apache.bookkeeper.bookie.LedgerStorage; import org.apache.bookkeeper.common.util.JsonUtil; import org.apache.bookkeeper.conf.ServerConfiguration; import org.apache.bookkeeper.http.HttpServer; @@ -29,6 +30,7 @@ import org.apache.bookkeeper.http.service.HttpEndpointService; import org.apache.bookkeeper.http.service.HttpServiceRequest; import org.apache.bookkeeper.http.service.HttpServiceResponse; import org.apache.bookkeeper.proto.BookieServer; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -61,40 +63,48 @@ public class TriggerGCService implements HttpEndpointService { @Override public HttpServiceResponse handle(HttpServiceRequest request) throws Exception { HttpServiceResponse response = new HttpServiceResponse(); + try { + if (HttpServer.Method.PUT == request.getMethod()) { + String requestBody = request.getBody(); + if (StringUtils.isBlank(requestBody)) { + bookieServer.getBookie().getLedgerStorage().forceGC(); + } else { + @SuppressWarnings("unchecked") + Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); + LedgerStorage ledgerStorage = bookieServer.getBookie().getLedgerStorage(); - if (HttpServer.Method.PUT == request.getMethod()) { - String requestBody = request.getBody(); - if (null == requestBody) { - bookieServer.getBookie().getLedgerStorage().forceGC(); - } else { - @SuppressWarnings("unchecked") - Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); - Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null); - Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null); - bookieServer.getBookie().getLedgerStorage().forceGC(forceMajor, forceMinor); - } + boolean forceMajor = Boolean.parseBoolean(configMap.getOrDefault("forceMajor", false).toString()); + boolean forceMinor = Boolean.parseBoolean(configMap.getOrDefault("forceMinor", false).toString()); + ledgerStorage.forceGC(forceMajor, forceMinor); + } - String output = "Triggered GC on BookieServer: " + bookieServer.toString(); - String jsonResponse = JsonUtil.toJson(output); - if (LOG.isDebugEnabled()) { - LOG.debug("output body:" + jsonResponse); - } - response.setBody(jsonResponse); - response.setCode(HttpServer.StatusCode.OK); - return response; - } else if (HttpServer.Method.GET == request.getMethod()) { - Boolean isInForceGC = bookieServer.getBookie().getLedgerStorage().isInForceGC(); - Pair<String, String> output = Pair.of("is_in_force_gc", isInForceGC.toString()); - String jsonResponse = JsonUtil.toJson(output); - if (LOG.isDebugEnabled()) { - LOG.debug("output body:" + jsonResponse); + String output = "Triggered GC on BookieServer: " + bookieServer.getBookieId(); + String jsonResponse = JsonUtil.toJson(output); + if (LOG.isDebugEnabled()) { + LOG.debug("output body:" + jsonResponse); + } + response.setBody(jsonResponse); + response.setCode(HttpServer.StatusCode.OK); + return response; + } else if (HttpServer.Method.GET == request.getMethod()) { + Boolean isInForceGC = bookieServer.getBookie().getLedgerStorage().isInForceGC(); + Pair<String, String> output = Pair.of("is_in_force_gc", isInForceGC.toString()); + String jsonResponse = JsonUtil.toJson(output); + if (LOG.isDebugEnabled()) { + LOG.debug("output body:" + jsonResponse); + } + response.setBody(jsonResponse); + response.setCode(HttpServer.StatusCode.OK); + return response; + } else { + response.setCode(HttpServer.StatusCode.METHOD_NOT_ALLOWED); + response.setBody("Not allowed method. Should be PUT to trigger GC, Or GET to get Force GC state."); + return response; } - response.setBody(jsonResponse); - response.setCode(HttpServer.StatusCode.OK); - return response; - } else { - response.setCode(HttpServer.StatusCode.NOT_FOUND); - response.setBody("Not found method. Should be PUT to trigger GC, Or GET to get Force GC state."); + } catch (Exception e) { + LOG.error("Failed to handle the request, method: {}, body: {} ", request.getMethod(), request.getBody(), e); + response.setCode(HttpServer.StatusCode.BAD_REQUEST); + response.setBody("Failed to handle the request, exception: " + e.getMessage()); return response; } } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/MockLedgerStorage.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/MockLedgerStorage.java index 06703966f2..648fc75296 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/MockLedgerStorage.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/MockLedgerStorage.java @@ -271,7 +271,7 @@ public class MockLedgerStorage implements LedgerStorage { } @Override - public void forceGC(Boolean forceMajor, Boolean forceMinor) { + public void forceGC(boolean forceMajor, boolean forceMinor) { LedgerStorage.super.forceGC(forceMajor, forceMinor); } diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/service/TriggerGCServiceTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/service/TriggerGCServiceTest.java new file mode 100644 index 0000000000..7a07ed59e2 --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/service/TriggerGCServiceTest.java @@ -0,0 +1,145 @@ +/* + * 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.server.http.service; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.RETURNS_DEEP_STUBS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import lombok.extern.slf4j.Slf4j; +import org.apache.bookkeeper.bookie.LedgerStorage; +import org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorage; +import org.apache.bookkeeper.conf.ServerConfiguration; +import org.apache.bookkeeper.http.HttpServer; +import org.apache.bookkeeper.http.service.HttpServiceRequest; +import org.apache.bookkeeper.http.service.HttpServiceResponse; +import org.apache.bookkeeper.proto.BookieServer; +import org.junit.Before; +import org.junit.Test; + + +/** + * Unit test for {@link TriggerGCService}. + */ +@Slf4j +public class TriggerGCServiceTest { + private TriggerGCService service; + private BookieServer mockBookieServer; + private LedgerStorage mockLedgerStorage; + + @Before + public void setup() { + this.mockBookieServer = mock(BookieServer.class, RETURNS_DEEP_STUBS); + this.mockLedgerStorage = mock(DbLedgerStorage.class); + when(mockBookieServer.getBookie().getLedgerStorage()).thenReturn(mockLedgerStorage); + when(mockLedgerStorage.isInForceGC()).thenReturn(false); + this.service = new TriggerGCService(new ServerConfiguration(), mockBookieServer); + } + + @Test + public void testHandleRequest() throws Exception { + + // test empty put body + HttpServiceRequest request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + HttpServiceResponse resp = service.handle(request); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test invalid put json body + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("test"); + resp = service.handle(request); + assertEquals(HttpServer.StatusCode.BAD_REQUEST.getValue(), resp.getStatusCode()); + assertEquals("Failed to handle the request, exception: Failed to deserialize Object from Json string", + resp.getBody()); + + // test forceMajor and forceMinor not set + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("{\"test\":1}"); + resp = service.handle(request); + verify(mockLedgerStorage, times(1)).forceGC(eq(true), eq(true)); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test forceMajor set, but forceMinor not set + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("{\"test\":1,\"forceMajor\":true}"); + resp = service.handle(request); + verify(mockLedgerStorage, times(2)).forceGC(eq(true), eq(true)); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test forceMajor set, but forceMinor not set + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("{\"test\":1,\"forceMajor\":\"true\"}"); + resp = service.handle(request); + verify(mockLedgerStorage, times(3)).forceGC(eq(true), eq(true)); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test forceMajor set to false, and forMinor not set + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("{\"test\":1,\"forceMajor\":false}"); + resp = service.handle(request); + verify(mockLedgerStorage, times(1)).forceGC(eq(false), eq(true)); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test forceMajor not set and forMinor set + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.PUT); + request.setBody("{\"test\":1,\"forceMinor\":true}"); + resp = service.handle(request); + verify(mockLedgerStorage, times(4)).forceGC(eq(true), eq(true)); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"", + resp.getBody()); + + // test get gc + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.GET); + resp = service.handle(request); + assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode()); + assertEquals("{\n \"is_in_force_gc\" : \"false\"\n}", resp.getBody()); + + // test invalid method type + request = new HttpServiceRequest(); + request.setMethod(HttpServer.Method.POST); + resp = service.handle(request); + assertEquals(HttpServer.StatusCode.METHOD_NOT_ALLOWED.getValue(), resp.getStatusCode()); + assertEquals("Not allowed method. Should be PUT to trigger GC, Or GET to get Force GC state.", + resp.getBody()); + } + +}
