Skip to content

Commit 8b3f782

Browse files
hangc0276zymap
hangc0276
authored and
zymap
committed
Fix trigger GC not work (#3998)
* Fix trigger gc not work (cherry picked from commit 147b5bf)
1 parent 14dbfd2 commit 8b3f782

File tree

9 files changed

+198
-40
lines changed

9 files changed

+198
-40
lines changed

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java

+2-3
Original file line numberDiff line numberDiff line change
@@ -292,12 +292,11 @@ public void enableForceGC() {
292292
}
293293
}
294294

295-
public void enableForceGC(Boolean forceMajor, Boolean forceMinor) {
295+
public void enableForceGC(boolean forceMajor, boolean forceMinor) {
296296
if (forceGarbageCollection.compareAndSet(false, true)) {
297297
LOG.info("Forced garbage collection triggered by thread: {}, forceMajor: {}, forceMinor: {}",
298298
Thread.currentThread().getName(), forceMajor, forceMinor);
299-
triggerGC(true, forceMajor == null ? suspendMajorCompaction.get() : !forceMajor,
300-
forceMinor == null ? suspendMinorCompaction.get() : !forceMinor);
299+
triggerGC(true, !forceMajor, !forceMinor);
301300
}
302301
}
303302

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ public void forceGC() {
260260
}
261261

262262
@Override
263-
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
263+
public void forceGC(boolean forceMajor, boolean forceMinor) {
264264
gcThread.enableForceGC(forceMajor, forceMinor);
265265
}
266266

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ default void forceGC() {
232232
/**
233233
* Force trigger Garbage Collection with forceMajor or forceMinor parameter.
234234
*/
235-
default void forceGC(Boolean forceMajor, Boolean forceMinor) {
235+
default void forceGC(boolean forceMajor, boolean forceMinor) {
236236
return;
237237
}
238238

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ public void forceGC() {
367367
}
368368

369369
@Override
370-
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
370+
public void forceGC(boolean forceMajor, boolean forceMinor) {
371371
interleavedLedgerStorage.forceGC(forceMajor, forceMinor);
372372
}
373373

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,7 @@ public void forceGC() {
506506
}
507507

508508
@Override
509-
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
509+
public void forceGC(boolean forceMajor, boolean forceMinor) {
510510
ledgerStorageList.stream().forEach(s -> s.forceGC(forceMajor, forceMinor));
511511
}
512512

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ public void forceGC() {
269269
}
270270

271271
@Override
272-
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
272+
public void forceGC(boolean forceMajor, boolean forceMinor) {
273273
gcThread.enableForceGC(forceMajor, forceMinor);
274274
}
275275

Diff for: ‎bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java

+43-31
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,15 @@
2222

2323
import java.util.HashMap;
2424
import java.util.Map;
25+
import org.apache.bookkeeper.bookie.LedgerStorage;
2526
import org.apache.bookkeeper.common.util.JsonUtil;
2627
import org.apache.bookkeeper.conf.ServerConfiguration;
2728
import org.apache.bookkeeper.http.HttpServer;
2829
import org.apache.bookkeeper.http.service.HttpEndpointService;
2930
import org.apache.bookkeeper.http.service.HttpServiceRequest;
3031
import org.apache.bookkeeper.http.service.HttpServiceResponse;
3132
import org.apache.bookkeeper.proto.BookieServer;
33+
import org.apache.commons.lang3.StringUtils;
3234
import org.apache.commons.lang3.tuple.Pair;
3335
import org.slf4j.Logger;
3436
import org.slf4j.LoggerFactory;
@@ -61,40 +63,50 @@ public TriggerGCService(ServerConfiguration conf, BookieServer bookieServer) {
6163
@Override
6264
public HttpServiceResponse handle(HttpServiceRequest request) throws Exception {
6365
HttpServiceResponse response = new HttpServiceResponse();
66+
try {
67+
if (HttpServer.Method.PUT == request.getMethod()) {
68+
String requestBody = request.getBody();
69+
if (StringUtils.isBlank(requestBody)) {
70+
bookieServer.getBookie().getLedgerStorage().forceGC();
71+
} else {
72+
@SuppressWarnings("unchecked")
73+
Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class);
74+
LedgerStorage ledgerStorage = bookieServer.getBookie().getLedgerStorage();
75+
boolean forceMajor = !ledgerStorage.isMajorGcSuspended();
76+
boolean forceMinor = !ledgerStorage.isMinorGcSuspended();
6477

65-
if (HttpServer.Method.PUT == request.getMethod()) {
66-
String requestBody = request.getBody();
67-
if (null == requestBody) {
68-
bookieServer.getBookie().getLedgerStorage().forceGC();
69-
} else {
70-
@SuppressWarnings("unchecked")
71-
Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class);
72-
Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null);
73-
Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null);
74-
bookieServer.getBookie().getLedgerStorage().forceGC(forceMajor, forceMinor);
75-
}
78+
forceMajor = Boolean.parseBoolean(configMap.getOrDefault("forceMajor", forceMajor).toString());
79+
forceMinor = Boolean.parseBoolean(configMap.getOrDefault("forceMinor", forceMinor).toString());
80+
ledgerStorage.forceGC(forceMajor, forceMinor);
81+
}
7682

77-
String output = "Triggered GC on BookieServer: " + bookieServer.toString();
78-
String jsonResponse = JsonUtil.toJson(output);
79-
if (LOG.isDebugEnabled()) {
80-
LOG.debug("output body:" + jsonResponse);
81-
}
82-
response.setBody(jsonResponse);
83-
response.setCode(HttpServer.StatusCode.OK);
84-
return response;
85-
} else if (HttpServer.Method.GET == request.getMethod()) {
86-
Boolean isInForceGC = bookieServer.getBookie().getLedgerStorage().isInForceGC();
87-
Pair<String, String> output = Pair.of("is_in_force_gc", isInForceGC.toString());
88-
String jsonResponse = JsonUtil.toJson(output);
89-
if (LOG.isDebugEnabled()) {
90-
LOG.debug("output body:" + jsonResponse);
83+
String output = "Triggered GC on BookieServer: " + bookieServer.getBookieId();
84+
String jsonResponse = JsonUtil.toJson(output);
85+
if (LOG.isDebugEnabled()) {
86+
LOG.debug("output body:" + jsonResponse);
87+
}
88+
response.setBody(jsonResponse);
89+
response.setCode(HttpServer.StatusCode.OK);
90+
return response;
91+
} else if (HttpServer.Method.GET == request.getMethod()) {
92+
Boolean isInForceGC = bookieServer.getBookie().getLedgerStorage().isInForceGC();
93+
Pair<String, String> output = Pair.of("is_in_force_gc", isInForceGC.toString());
94+
String jsonResponse = JsonUtil.toJson(output);
95+
if (LOG.isDebugEnabled()) {
96+
LOG.debug("output body:" + jsonResponse);
97+
}
98+
response.setBody(jsonResponse);
99+
response.setCode(HttpServer.StatusCode.OK);
100+
return response;
101+
} else {
102+
response.setCode(HttpServer.StatusCode.METHOD_NOT_ALLOWED);
103+
response.setBody("Not allowed method. Should be PUT to trigger GC, Or GET to get Force GC state.");
104+
return response;
91105
}
92-
response.setBody(jsonResponse);
93-
response.setCode(HttpServer.StatusCode.OK);
94-
return response;
95-
} else {
96-
response.setCode(HttpServer.StatusCode.NOT_FOUND);
97-
response.setBody("Not found method. Should be PUT to trigger GC, Or GET to get Force GC state.");
106+
} catch (Exception e) {
107+
LOG.error("Failed to handle the request, method: {}, body: {} ", request.getMethod(), request.getBody(), e);
108+
response.setCode(HttpServer.StatusCode.BAD_REQUEST);
109+
response.setBody("Failed to handle the request, exception: " + e.getMessage());
98110
return response;
99111
}
100112
}

Diff for: ‎bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/MockLedgerStorage.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ public void forceGC() {
271271
}
272272

273273
@Override
274-
public void forceGC(Boolean forceMajor, Boolean forceMinor) {
274+
public void forceGC(boolean forceMajor, boolean forceMinor) {
275275
CompactableLedgerStorage.super.forceGC(forceMajor, forceMinor);
276276
}
277277

Diff for: ‎bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/service/TriggerGCServiceTest.java

+147
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.bookkeeper.server.http.service;
20+
21+
import static org.junit.Assert.assertEquals;
22+
import static org.mockito.ArgumentMatchers.eq;
23+
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
24+
import static org.mockito.Mockito.mock;
25+
import static org.mockito.Mockito.times;
26+
import static org.mockito.Mockito.verify;
27+
import static org.mockito.Mockito.when;
28+
29+
import lombok.extern.slf4j.Slf4j;
30+
import org.apache.bookkeeper.bookie.LedgerStorage;
31+
import org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorage;
32+
import org.apache.bookkeeper.conf.ServerConfiguration;
33+
import org.apache.bookkeeper.http.HttpServer;
34+
import org.apache.bookkeeper.http.service.HttpServiceRequest;
35+
import org.apache.bookkeeper.http.service.HttpServiceResponse;
36+
import org.apache.bookkeeper.proto.BookieServer;
37+
import org.junit.Before;
38+
import org.junit.Test;
39+
40+
41+
/**
42+
* Unit test for {@link TriggerGCService}.
43+
*/
44+
@Slf4j
45+
public class TriggerGCServiceTest {
46+
private TriggerGCService service;
47+
private BookieServer mockBookieServer;
48+
private LedgerStorage mockLedgerStorage;
49+
50+
@Before
51+
public void setup() {
52+
this.mockBookieServer = mock(BookieServer.class, RETURNS_DEEP_STUBS);
53+
this.mockLedgerStorage = mock(DbLedgerStorage.class);
54+
when(mockBookieServer.getBookie().getLedgerStorage()).thenReturn(mockLedgerStorage);
55+
when(mockLedgerStorage.isInForceGC()).thenReturn(false);
56+
when(mockLedgerStorage.isMajorGcSuspended()).thenReturn(false);
57+
when(mockLedgerStorage.isMinorGcSuspended()).thenReturn(false);
58+
this.service = new TriggerGCService(new ServerConfiguration(), mockBookieServer);
59+
}
60+
61+
@Test
62+
public void testHandleRequest() throws Exception {
63+
64+
// test empty put body
65+
HttpServiceRequest request = new HttpServiceRequest();
66+
request.setMethod(HttpServer.Method.PUT);
67+
HttpServiceResponse resp = service.handle(request);
68+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
69+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
70+
resp.getBody());
71+
72+
// test invalid put json body
73+
request = new HttpServiceRequest();
74+
request.setMethod(HttpServer.Method.PUT);
75+
request.setBody("test");
76+
resp = service.handle(request);
77+
assertEquals(HttpServer.StatusCode.BAD_REQUEST.getValue(), resp.getStatusCode());
78+
assertEquals("Failed to handle the request, exception: Failed to deserialize Object from Json string",
79+
resp.getBody());
80+
81+
// test forceMajor and forceMinor not set
82+
request = new HttpServiceRequest();
83+
request.setMethod(HttpServer.Method.PUT);
84+
request.setBody("{\"test\":1}");
85+
resp = service.handle(request);
86+
verify(mockLedgerStorage, times(1)).forceGC(eq(true), eq(true));
87+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
88+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
89+
resp.getBody());
90+
91+
// test forceMajor set, but forceMinor not set
92+
request = new HttpServiceRequest();
93+
request.setMethod(HttpServer.Method.PUT);
94+
request.setBody("{\"test\":1,\"forceMajor\":true}");
95+
resp = service.handle(request);
96+
verify(mockLedgerStorage, times(2)).forceGC(eq(true), eq(true));
97+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
98+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
99+
resp.getBody());
100+
101+
// test forceMajor set, but forceMinor not set
102+
request = new HttpServiceRequest();
103+
request.setMethod(HttpServer.Method.PUT);
104+
request.setBody("{\"test\":1,\"forceMajor\":\"true\"}");
105+
resp = service.handle(request);
106+
verify(mockLedgerStorage, times(3)).forceGC(eq(true), eq(true));
107+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
108+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
109+
resp.getBody());
110+
111+
// test forceMajor set to false, and forMinor not set
112+
request = new HttpServiceRequest();
113+
request.setMethod(HttpServer.Method.PUT);
114+
request.setBody("{\"test\":1,\"forceMajor\":false}");
115+
resp = service.handle(request);
116+
verify(mockLedgerStorage, times(1)).forceGC(eq(false), eq(true));
117+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
118+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
119+
resp.getBody());
120+
121+
// test forceMajor not set and forMinor set
122+
request = new HttpServiceRequest();
123+
request.setMethod(HttpServer.Method.PUT);
124+
request.setBody("{\"test\":1,\"forceMinor\":true}");
125+
resp = service.handle(request);
126+
verify(mockLedgerStorage, times(4)).forceGC(eq(true), eq(true));
127+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
128+
assertEquals("\"Triggered GC on BookieServer: " + mockBookieServer.getBookieId() + "\"",
129+
resp.getBody());
130+
131+
// test get gc
132+
request = new HttpServiceRequest();
133+
request.setMethod(HttpServer.Method.GET);
134+
resp = service.handle(request);
135+
assertEquals(HttpServer.StatusCode.OK.getValue(), resp.getStatusCode());
136+
assertEquals("{\n \"is_in_force_gc\" : \"false\"\n}", resp.getBody());
137+
138+
// test invalid method type
139+
request = new HttpServiceRequest();
140+
request.setMethod(HttpServer.Method.POST);
141+
resp = service.handle(request);
142+
assertEquals(HttpServer.StatusCode.METHOD_NOT_ALLOWED.getValue(), resp.getStatusCode());
143+
assertEquals("Not allowed method. Should be PUT to trigger GC, Or GET to get Force GC state.",
144+
resp.getBody());
145+
}
146+
147+
}

0 commit comments

Comments
 (0)