Copilot commented on code in PR #9663: URL: https://github.com/apache/ozone/pull/9663#discussion_r2792337174
########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.hadoop.ozone.om.service; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import org.apache.hadoop.hdds.utils.db.RDBStore; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; +import org.apache.hadoop.hdds.utils.db.managed.ManagedCompactRangeOptions; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Utility class for compacting OM RocksDB column families. + */ +public final class CompactDBUtil { + private static final Logger LOG = + LoggerFactory.getLogger(CompactDBUtil.class); + + private CompactDBUtil() { + } + + public static void compactTable(OMMetadataManager omMetadataManager, + String tableName) throws IOException { + long startTime = Time.monotonicNow(); + LOG.info("Compacting column family: {}", tableName); + try (ManagedCompactRangeOptions options = new ManagedCompactRangeOptions()) { + options.setBottommostLevelCompaction( + ManagedCompactRangeOptions.BottommostLevelCompaction.kForce); + options.setExclusiveManualCompaction(true); + RocksDatabase rocksDatabase = + ((RDBStore) omMetadataManager.getStore()).getDb(); + + try { + RocksDatabase.ColumnFamily columnFamily = + rocksDatabase.getColumnFamily(tableName); + rocksDatabase.compactRange(columnFamily, null, null, options); + LOG.info("Compaction of column family: {} completed in {} ms", + tableName, Time.monotonicNow() - startTime); + } catch (NullPointerException ex) { + LOG.error("Unable to trigger compaction for \"{}\". Column family not found ", + tableName); + throw new IOException("Column family \"" + tableName + "\" not found."); + } + } + } + + public static CompletableFuture<Void> compactTableAsync(OMMetadataManager metadataManager, String tableName) { + return CompletableFuture.runAsync(() -> { + try { + compactTable(metadataManager, tableName); + } catch (Exception e) { + LOG.warn("Failed to compact column family: {}", tableName, e); Review Comment: `compactTableAsync` catches all exceptions and only logs, so the returned `CompletableFuture` will always complete normally even when compaction fails. This makes it impossible for callers (eg `CompactionService.compactTableAsync`) to detect failure. Consider letting exceptions propagate so the future completes exceptionally, or explicitly completing the future exceptionally after logging. ```suggestion LOG.warn("Failed to compact column family: {}", tableName, e); throw new RuntimeException("Compaction failed for column family: " + tableName, e); ``` ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java: ########## @@ -5381,7 +5381,7 @@ public void checkFeatureEnabled(OzoneManagerVersion feature) throws OMException public void compactOMDB(String columnFamily) throws IOException { checkAdminUserPrivilege("compact column family " + columnFamily); - new CompactDBService(this).compact(columnFamily); + CompactDBUtil.compactTableAsync(metadataManager, columnFamily); } Review Comment: `compactOMDB` triggers an async compaction and drops the returned future. Since `CompactDBUtil.compactTableAsync` currently logs and swallows failures, the admin RPC will report success once the request is issued even if the compaction later fails. If that’s intended, consider at least attaching a completion handler to log success/failure clearly; otherwise, propagate failures by returning/completing the future exceptionally in the async path. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.hadoop.ozone.om.service; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import org.apache.hadoop.hdds.utils.db.RDBStore; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; +import org.apache.hadoop.hdds.utils.db.managed.ManagedCompactRangeOptions; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Utility class for compacting OM RocksDB column families. + */ +public final class CompactDBUtil { + private static final Logger LOG = + LoggerFactory.getLogger(CompactDBUtil.class); + + private CompactDBUtil() { + } + + public static void compactTable(OMMetadataManager omMetadataManager, + String tableName) throws IOException { + long startTime = Time.monotonicNow(); + LOG.info("Compacting column family: {}", tableName); + try (ManagedCompactRangeOptions options = new ManagedCompactRangeOptions()) { + options.setBottommostLevelCompaction( + ManagedCompactRangeOptions.BottommostLevelCompaction.kForce); + options.setExclusiveManualCompaction(true); + RocksDatabase rocksDatabase = + ((RDBStore) omMetadataManager.getStore()).getDb(); + + try { + RocksDatabase.ColumnFamily columnFamily = + rocksDatabase.getColumnFamily(tableName); + rocksDatabase.compactRange(columnFamily, null, null, options); + LOG.info("Compaction of column family: {} completed in {} ms", + tableName, Time.monotonicNow() - startTime); + } catch (NullPointerException ex) { + LOG.error("Unable to trigger compaction for \"{}\". Column family not found ", + tableName); + throw new IOException("Column family \"" + tableName + "\" not found."); + } Review Comment: `compactTable` uses a `NullPointerException` catch to detect a missing column family. This can mask real NPEs from other causes and makes failures harder to diagnose. Prefer explicitly checking the result of `rocksDatabase.getColumnFamily(tableName)` for null and throwing an `IOException` (ideally preserving the original cause) when the column family is not found. ```suggestion RocksDatabase.ColumnFamily columnFamily = rocksDatabase.getColumnFamily(tableName); if (columnFamily == null) { LOG.error("Unable to trigger compaction for \"{}\". Column family not found ", tableName); throw new IOException("Column family \"" + tableName + "\" not found."); } rocksDatabase.compactRange(columnFamily, null, null, options); LOG.info("Compaction of column family: {} completed in {} ms", tableName, Time.monotonicNow() - startTime); ``` ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.hadoop.ozone.om.service; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import org.apache.hadoop.hdds.utils.db.RDBStore; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; +import org.apache.hadoop.hdds.utils.db.managed.ManagedCompactRangeOptions; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Utility class for compacting OM RocksDB column families. + */ +public final class CompactDBUtil { + private static final Logger LOG = + LoggerFactory.getLogger(CompactDBUtil.class); + + private CompactDBUtil() { + } + + public static void compactTable(OMMetadataManager omMetadataManager, + String tableName) throws IOException { + long startTime = Time.monotonicNow(); + LOG.info("Compacting column family: {}", tableName); + try (ManagedCompactRangeOptions options = new ManagedCompactRangeOptions()) { + options.setBottommostLevelCompaction( + ManagedCompactRangeOptions.BottommostLevelCompaction.kForce); + options.setExclusiveManualCompaction(true); + RocksDatabase rocksDatabase = + ((RDBStore) omMetadataManager.getStore()).getDb(); Review Comment: Access of [element](1) annotated with VisibleForTesting found in production code. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactionService.java: ########## @@ -134,25 +131,21 @@ private boolean shouldRun() { return !suspended.get(); } + /** + * Compact a specific table asynchronously. This method returns immediately + * with a CompletableFuture that completes when the compaction finishes. + * This is useful for on-demand compaction requests (e.g., via admin RPC) + * where the caller doesn't need to wait for completion. + * + * @param tableName the name of the table to compact + * @return CompletableFuture that completes when compaction finishes + */ + public CompletableFuture<Void> compactTableAsync(String tableName) { + return CompactDBUtil.compactTableAsync(omMetadataManager, tableName); + } Review Comment: `CompactionService.compactTableAsync` delegates to `CompactDBUtil.compactTableAsync`, which currently runs on the common ForkJoinPool and may run concurrently with this service’s own scheduled compaction thread. For on-demand compaction, consider scheduling the work on this `BackgroundService`'s executor (serializing with existing compactions) to avoid uncontrolled parallel manual compactions and avoid using the common pool. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.hadoop.ozone.om.service; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import org.apache.hadoop.hdds.utils.db.RDBStore; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; +import org.apache.hadoop.hdds.utils.db.managed.ManagedCompactRangeOptions; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Utility class for compacting OM RocksDB column families. + */ +public final class CompactDBUtil { + private static final Logger LOG = + LoggerFactory.getLogger(CompactDBUtil.class); + + private CompactDBUtil() { + } + + public static void compactTable(OMMetadataManager omMetadataManager, + String tableName) throws IOException { + long startTime = Time.monotonicNow(); + LOG.info("Compacting column family: {}", tableName); + try (ManagedCompactRangeOptions options = new ManagedCompactRangeOptions()) { + options.setBottommostLevelCompaction( + ManagedCompactRangeOptions.BottommostLevelCompaction.kForce); + options.setExclusiveManualCompaction(true); + RocksDatabase rocksDatabase = + ((RDBStore) omMetadataManager.getStore()).getDb(); + + try { + RocksDatabase.ColumnFamily columnFamily = + rocksDatabase.getColumnFamily(tableName); + rocksDatabase.compactRange(columnFamily, null, null, options); + LOG.info("Compaction of column family: {} completed in {} ms", + tableName, Time.monotonicNow() - startTime); + } catch (NullPointerException ex) { + LOG.error("Unable to trigger compaction for \"{}\". Column family not found ", + tableName); + throw new IOException("Column family \"" + tableName + "\" not found."); + } + } + } + + public static CompletableFuture<Void> compactTableAsync(OMMetadataManager metadataManager, String tableName) { + return CompletableFuture.runAsync(() -> { + try { + compactTable(metadataManager, tableName); + } catch (Exception e) { + LOG.warn("Failed to compact column family: {}", tableName, e); + } + }); Review Comment: `CompletableFuture.runAsync(...)` here uses the common ForkJoinPool by default. RocksDB compaction can be long-running/blocking, so running it on the common pool can starve other unrelated async work and is hard to size/control operationally. Prefer using a dedicated executor (or accepting an `Executor` parameter) so compactions run on an OM/compaction-specific thread pool. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.hadoop.ozone.om.service; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import org.apache.hadoop.hdds.utils.db.RDBStore; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; +import org.apache.hadoop.hdds.utils.db.managed.ManagedCompactRangeOptions; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Utility class for compacting OM RocksDB column families. + */ +public final class CompactDBUtil { + private static final Logger LOG = + LoggerFactory.getLogger(CompactDBUtil.class); + + private CompactDBUtil() { + } + + public static void compactTable(OMMetadataManager omMetadataManager, + String tableName) throws IOException { + long startTime = Time.monotonicNow(); + LOG.info("Compacting column family: {}", tableName); + try (ManagedCompactRangeOptions options = new ManagedCompactRangeOptions()) { + options.setBottommostLevelCompaction( + ManagedCompactRangeOptions.BottommostLevelCompaction.kForce); + options.setExclusiveManualCompaction(true); + RocksDatabase rocksDatabase = Review Comment: This refactor changes admin-triggered compaction behavior vs the removed `CompactDBService`: `compactTable` now sets `options.setExclusiveManualCompaction(true)`. If `CompactDBService` previously allowed manual compactions to run without this exclusivity, this could introduce unexpected blocking/serialization with other manual compactions. Please confirm this is intended (or make the exclusivity configurable / only apply it for the background compaction path). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
