Skip to content

Commit

Permalink
Fix trigger GC not work (#3998)
Browse files Browse the repository at this point in the history
* Fix trigger gc not work

(cherry picked from commit 147b5bf)
  • Loading branch information
hangc0276 authored and zymap committed Jun 26, 2023
1 parent 14dbfd2 commit 8b3f782
Show file tree
Hide file tree
Showing 9 changed files with 198 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -292,12 +292,11 @@ public void enableForceGC() {
}
}

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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public void forceGC() {
}

@Override
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
public void forceGC(boolean forceMajor, boolean forceMinor) {
gcThread.enableForceGC(forceMajor, forceMinor);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ default void forceGC() {
/**
* Force trigger Garbage Collection with forceMajor or forceMinor parameter.
*/
default void forceGC(Boolean forceMajor, Boolean forceMinor) {
default void forceGC(boolean forceMajor, boolean forceMinor) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ public void forceGC() {
}

@Override
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
public void forceGC(boolean forceMajor, boolean forceMinor) {
interleavedLedgerStorage.forceGC(forceMajor, forceMinor);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ public void forceGC() {
}

@Override
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
public void forceGC(boolean forceMajor, boolean forceMinor) {
ledgerStorageList.stream().forEach(s -> s.forceGC(forceMajor, forceMinor));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ public void forceGC() {
}

@Override
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
public void forceGC(boolean forceMajor, boolean forceMinor) {
gcThread.enableForceGC(forceMajor, forceMinor);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@

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;
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;
Expand Down Expand Up @@ -61,40 +63,50 @@ public TriggerGCService(ServerConfiguration conf, BookieServer bookieServer) {
@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();
boolean forceMajor = !ledgerStorage.isMajorGcSuspended();
boolean forceMinor = !ledgerStorage.isMinorGcSuspended();

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);
}
forceMajor = Boolean.parseBoolean(configMap.getOrDefault("forceMajor", forceMajor).toString());
forceMinor = Boolean.parseBoolean(configMap.getOrDefault("forceMinor", forceMinor).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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public void forceGC() {
}

@Override
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
public void forceGC(boolean forceMajor, boolean forceMinor) {
CompactableLedgerStorage.super.forceGC(forceMajor, forceMinor);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
* 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);
when(mockLedgerStorage.isMajorGcSuspended()).thenReturn(false);
when(mockLedgerStorage.isMinorGcSuspended()).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());
}

}

0 comments on commit 8b3f782

Please sign in to comment.