hangc0276 commented on code in PR #3802: URL: https://github.com/apache/bookkeeper/pull/3802#discussion_r1118272251
########## bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerLocationCompactService.java: ########## @@ -0,0 +1,102 @@ +/* + * 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 com.google.common.base.Preconditions.checkNotNull; + +import java.util.HashMap; +import java.util.Map; +import org.apache.bookkeeper.common.util.JsonUtil; +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; + +/** + * HttpEndpointService that handle force trigger entry location compact requests. + * + * <p>The PUT method will trigger entry location compact on current bookie. + * + * <p>The GET method will get the entry location compact running or not. + * Output would be like: + * { + * "is_entry_location_compact" : "false" + * } + */ + +public class TriggerLocationCompactService implements HttpEndpointService { + + static final Logger LOG = LoggerFactory.getLogger(TriggerLocationCompactService.class); + + private final BookieServer bookieServer; + + public TriggerLocationCompactService(BookieServer bookieServer) { + this.bookieServer = checkNotNull(bookieServer); + } + + @Override + public HttpServiceResponse handle(HttpServiceRequest request) throws Exception { + HttpServiceResponse response = new HttpServiceResponse(); + + if (HttpServer.Method.PUT.equals(request.getMethod())) { + String requestBody = request.getBody(); + String output = "Not trigger Entry Location RocksDB compact."; + if (StringUtils.isNotBlank(requestBody)) { + @SuppressWarnings("unchecked") + Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); Review Comment: If we configured multiple ledger directories, we will trigger all the location index RocksDB table compaction at the same time. Do we need to provide the ability to trigger compaction for specific ledger directory's location index RocksDB table compaction? ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java: ########## @@ -542,6 +542,16 @@ public boolean isMinorGcSuspended() { return ledgerStorageList.stream().allMatch(SingleDirectoryDbLedgerStorage::isMinorGcSuspended); } + @Override + public void entryLocationCompact() { + ledgerStorageList.stream().forEach(SingleDirectoryDbLedgerStorage::entryLocationCompact); Review Comment: use `ledgerStorageList.forEach(SingleDirectoryDbLedgerStorage::entryLocationCompact);` ? ########## bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/TestHttpService.java: ########## @@ -1087,4 +1087,28 @@ public void testSuspendCompaction() throws Exception { assertEquals(responseMap7.get("isMajorGcSuspended"), "false"); assertEquals(responseMap7.get("isMinorGcSuspended"), "false"); } + + @Test + public void testTriggerEntryLocationCompactService() throws Exception { + HttpEndpointService triggerEntryLocationCompactService = bkHttpServiceProvider + .provideHttpEndpointService(HttpServer.ApiType.TRIGGER_ENTRY_LOCATION_COMPACT); + + //1. Put, should return OK + HttpServiceRequest request1 = new HttpServiceRequest(null, HttpServer.Method.PUT, null); + HttpServiceResponse response1 = triggerEntryLocationCompactService.handle(request1); + assertEquals(HttpServer.StatusCode.OK.getValue(), response1.getStatusCode()); + LOG.info("Get response: {}", response1.getBody()); + + //2. GET, should return OK + HttpServiceRequest request2 = new HttpServiceRequest(null, HttpServer.Method.GET, null); Review Comment: Trigger compaction first and send get request to get the result, and please cover multiple ledger directory case. ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerLocationCompactService.java: ########## @@ -0,0 +1,102 @@ +/* + * 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 com.google.common.base.Preconditions.checkNotNull; + +import java.util.HashMap; +import java.util.Map; +import org.apache.bookkeeper.common.util.JsonUtil; +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; + +/** + * HttpEndpointService that handle force trigger entry location compact requests. + * + * <p>The PUT method will trigger entry location compact on current bookie. + * + * <p>The GET method will get the entry location compact running or not. + * Output would be like: + * { + * "is_entry_location_compact" : "false" + * } + */ + +public class TriggerLocationCompactService implements HttpEndpointService { + + static final Logger LOG = LoggerFactory.getLogger(TriggerLocationCompactService.class); + + private final BookieServer bookieServer; + + public TriggerLocationCompactService(BookieServer bookieServer) { + this.bookieServer = checkNotNull(bookieServer); + } + + @Override + public HttpServiceResponse handle(HttpServiceRequest request) throws Exception { + HttpServiceResponse response = new HttpServiceResponse(); + + if (HttpServer.Method.PUT.equals(request.getMethod())) { + String requestBody = request.getBody(); + String output = "Not trigger Entry Location RocksDB compact."; + if (StringUtils.isNotBlank(requestBody)) { + @SuppressWarnings("unchecked") + Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); + Boolean isEntryLocationCompact = (Boolean) configMap.getOrDefault("entryLocationRocksDBCompact", false); + boolean isCompactRunning = bookieServer.getBookie().getLedgerStorage().isEntryLocationCompact(); + if (isEntryLocationCompact && !isCompactRunning){ Review Comment: need one blank ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/EntryLocationIndex.java: ########## @@ -189,6 +190,21 @@ public void delete(long ledgerId) throws IOException { deletedLedgers.add(ledgerId); } + public void compact() { + try { + isCompact = true; Review Comment: `compacting`? ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerLocationCompactService.java: ########## @@ -0,0 +1,102 @@ +/* + * 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 com.google.common.base.Preconditions.checkNotNull; + +import java.util.HashMap; +import java.util.Map; +import org.apache.bookkeeper.common.util.JsonUtil; +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; + +/** + * HttpEndpointService that handle force trigger entry location compact requests. + * + * <p>The PUT method will trigger entry location compact on current bookie. + * + * <p>The GET method will get the entry location compact running or not. + * Output would be like: + * { + * "is_entry_location_compact" : "false" + * } + */ + +public class TriggerLocationCompactService implements HttpEndpointService { + + static final Logger LOG = LoggerFactory.getLogger(TriggerLocationCompactService.class); + + private final BookieServer bookieServer; + + public TriggerLocationCompactService(BookieServer bookieServer) { + this.bookieServer = checkNotNull(bookieServer); + } + + @Override + public HttpServiceResponse handle(HttpServiceRequest request) throws Exception { + HttpServiceResponse response = new HttpServiceResponse(); + + if (HttpServer.Method.PUT.equals(request.getMethod())) { + String requestBody = request.getBody(); + String output = "Not trigger Entry Location RocksDB compact."; + if (StringUtils.isNotBlank(requestBody)) { + @SuppressWarnings("unchecked") + Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); Review Comment: If the JsonUtil.fromJson throws an exception, do we need to catch this exception and return an error response to the client? ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/EntryLocationIndex.java: ########## @@ -189,6 +190,21 @@ public void delete(long ledgerId) throws IOException { deletedLedgers.add(ledgerId); } + public void compact() { + try { + isCompact = true; + locationsDb.compact(); + } catch (IOException e) { Review Comment: Maybe we can remove the catch here due to we have caught the exception outside. ########## bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerLocationCompactService.java: ########## @@ -0,0 +1,102 @@ +/* + * 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 com.google.common.base.Preconditions.checkNotNull; + +import java.util.HashMap; +import java.util.Map; +import org.apache.bookkeeper.common.util.JsonUtil; +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; + +/** + * HttpEndpointService that handle force trigger entry location compact requests. + * + * <p>The PUT method will trigger entry location compact on current bookie. + * + * <p>The GET method will get the entry location compact running or not. + * Output would be like: + * { + * "is_entry_location_compact" : "false" + * } + */ + +public class TriggerLocationCompactService implements HttpEndpointService { + + static final Logger LOG = LoggerFactory.getLogger(TriggerLocationCompactService.class); + + private final BookieServer bookieServer; + + public TriggerLocationCompactService(BookieServer bookieServer) { + this.bookieServer = checkNotNull(bookieServer); + } + + @Override + public HttpServiceResponse handle(HttpServiceRequest request) throws Exception { + HttpServiceResponse response = new HttpServiceResponse(); + + if (HttpServer.Method.PUT.equals(request.getMethod())) { + String requestBody = request.getBody(); + String output = "Not trigger Entry Location RocksDB compact."; + if (StringUtils.isNotBlank(requestBody)) { + @SuppressWarnings("unchecked") + Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); + Boolean isEntryLocationCompact = (Boolean) configMap.getOrDefault("entryLocationRocksDBCompact", false); + boolean isCompactRunning = bookieServer.getBookie().getLedgerStorage().isEntryLocationCompact(); + if (isEntryLocationCompact && !isCompactRunning){ + bookieServer.getBookie().getLedgerStorage().entryLocationCompact(); + output = "Triggered Entry Location RocksDB compact on bookie: " + bookieServer.getBookieId(); + } else if (isCompactRunning) { + output = "Entry Location RocksDB compact already running on bookie: " + 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 isCompactRunning = bookieServer.getBookie().getLedgerStorage().isEntryLocationCompact(); Review Comment: We'd better show the detailed compacting info for each ledger directory instead of the total one ########## bookkeeper-server/src/test/java/org/apache/bookkeeper/server/http/TestHttpService.java: ########## @@ -1087,4 +1087,28 @@ public void testSuspendCompaction() throws Exception { assertEquals(responseMap7.get("isMajorGcSuspended"), "false"); assertEquals(responseMap7.get("isMinorGcSuspended"), "false"); } + + @Test + public void testTriggerEntryLocationCompactService() throws Exception { + HttpEndpointService triggerEntryLocationCompactService = bkHttpServiceProvider + .provideHttpEndpointService(HttpServer.ApiType.TRIGGER_ENTRY_LOCATION_COMPACT); + + //1. Put, should return OK + HttpServiceRequest request1 = new HttpServiceRequest(null, HttpServer.Method.PUT, null); Review Comment: We need to test the invalid parameter cases. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
