Copilot commented on code in PR #9155:
URL: https://github.com/apache/ozone/pull/9155#discussion_r2483182840
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OMAdminProtocolClientSideImpl.java:
##########
@@ -232,6 +234,25 @@ public void compactOMDB(String columnFamily) throws
IOException {
}
}
+ @Override
+ public boolean triggerSnapshotDefrag(boolean noWait) throws IOException {
+ TriggerSnapshotDefragRequest request =
TriggerSnapshotDefragRequest.newBuilder()
+ .setNoWait(noWait)
+ .build();
+ TriggerSnapshotDefragResponse response;
+ try {
+ response = rpcProxy.triggerSnapshotDefrag(NULL_RPC_CONTROLLER, request);
+ } catch (ServiceException e) {
+ throw ProtobufHelper.getRemoteException(e);
+ }
+ if (!response.getSuccess()) {
+ throwException("Request to trigger snapshot defragmentation" +
+ ", sent to " + omPrintInfo + " failed with error: " +
+ response.getErrorMsg());
+ }
+ return response.hasResult() ? response.getResult() : true;
Review Comment:
The default return value of 'true' when result is not present could mask
failures. When noWait=true, the server always sets result=true (line 3606 in
OzoneManager.java), so hasResult() should always be true in that case. When
noWait=false and result is missing, it likely indicates an error condition.
Consider returning false as the default or throwing an exception to make the
failure explicit.
```suggestion
if (response.hasResult()) {
return response.getResult();
} else {
throwException("Missing result in TriggerSnapshotDefragResponse from "
+ omPrintInfo +
". This likely indicates a server error.");
// Unreachable, but required for compilation
return false;
}
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/om/snapshot/DefragSubCommand.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.admin.om.snapshot;
+
+import java.io.IOException;
+import java.util.concurrent.Callable;
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.admin.om.OMAdmin;
+import org.apache.hadoop.ozone.om.helpers.OMNodeDetails;
+import org.apache.hadoop.ozone.om.protocolPB.OMAdminProtocolClientSideImpl;
+import org.apache.hadoop.security.UserGroupInformation;
+import picocli.CommandLine;
+
+/**
+ * Handler of ozone admin om snapshot defrag command.
+ */
[email protected](
+ name = "defrag",
+ description = "Triggers the Snapshot Defragmentation Service to run " +
+ "immediately. This command manually initiates the snapshot " +
+ "defragmentation process which compacts snapshot data and removes " +
+ "fragmentation to improve storage efficiency. " +
+ "This command works only on OzoneManager HA cluster.",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class
+)
+public class DefragSubCommand implements Callable<Void> {
+
+ @CommandLine.ParentCommand
+ private SnapshotSubCommand parent;
+
+ @CommandLine.Option(
+ names = {"-id", "--service-id"},
+ description = "Ozone Manager Service ID"
+ )
+ private String omServiceId;
+
+ @CommandLine.Option(
+ names = {"--node-id"},
+ description = "NodeID of the OM to trigger snapshot defragmentation on.",
+ required = false
+ )
+ private String nodeId;
+
+ @CommandLine.Option(
+ names = {"--no-wait"},
+ description = "Do not wait for the defragmentation task to complete. " +
+ "The command will return immediately after triggering the task.",
+ defaultValue = "false"
+ )
+ private boolean noWait;
+
+ @Override
+ public Void call() throws Exception {
+ // Navigate up to get OMAdmin
+ OMAdmin omAdmin = getOMAdmin();
+ OzoneConfiguration conf = omAdmin.getParent().getOzoneConf();
+ OMNodeDetails omNodeDetails = OMNodeDetails.getOMNodeDetailsFromConf(
+ conf, omServiceId, nodeId);
+
+ if (omNodeDetails == null) {
+ System.err.println("Error: OMNodeDetails could not be determined with
given " +
+ "service ID and node ID.");
+ return null;
+ }
+
+ try (OMAdminProtocolClientSideImpl omAdminProtocolClient =
createClient(conf, omNodeDetails)) {
+ execute(omAdminProtocolClient);
+ } catch (IOException ex) {
+ System.err.println("Failed to trigger snapshot defragmentation: " +
+ ex.getMessage());
+ throw ex;
+ }
+
+ return null;
+ }
+
+ protected OMAdminProtocolClientSideImpl createClient(
+ OzoneConfiguration conf, OMNodeDetails omNodeDetails) throws IOException
{
+ return OMAdminProtocolClientSideImpl.createProxyForSingleOM(conf,
+ UserGroupInformation.getCurrentUser(), omNodeDetails);
+ }
+
+ protected void execute(OMAdminProtocolClientSideImpl omAdminProtocolClient)
+ throws IOException {
+ System.out.println("Triggering Snapshot Defrag Service ...");
+ boolean result = omAdminProtocolClient.triggerSnapshotDefrag(noWait);
+
+ if (noWait) {
+ System.out.println("Snapshot defragmentation task has been triggered " +
+ "successfully and is running in the background.");
+ } else {
+ if (result) {
+ System.out.println("Snapshot defragmentation completed successfully.");
+ } else {
+ System.out.println("Snapshot defragmentation task failed or was
interrupted.");
+ }
+ }
+ }
+
+ private OMAdmin getOMAdmin() {
+ // The parent hierarchy is: DefragSubCommand -> SnapshotSubCommand ->
OMAdmin
+ // We need to use reflection or add a parent reference in
SnapshotSubCommand
+ // For now, let's add a reference in SnapshotSubCommand
Review Comment:
The comment on lines 119-120 describes an alternative approach using
reflection, but this was not the approach taken. This comment should be removed
as it describes a discarded design option rather than explaining the current
implementation.
```suggestion
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDefragAdmin.java:
##########
@@ -0,0 +1,219 @@
+/*
+ * 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.snapshot;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.List;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.utils.IOUtils;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.MiniOzoneHAClusterImpl;
+import org.apache.hadoop.ozone.admin.OzoneAdmin;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.om.OMConfigKeys;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+
+/**
+ * Integration test for 'ozone admin om snapshot defrag' command.
+ * Tests that the defrag command can be successfully triggered on any OM
+ * (leader or follower) in an HA cluster.
+ */
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public class TestSnapshotDefragAdmin {
+
+ private static MiniOzoneHAClusterImpl cluster;
+ private static OzoneClient client;
+ private static String omServiceId;
+
+ @BeforeAll
+ public static void init() throws Exception {
+ OzoneConfiguration conf = new OzoneConfiguration();
+ conf.setBoolean(OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY, true);
+ // Enable snapshot defrag service
+ conf.setInt(OMConfigKeys.OZONE_SNAPSHOT_DEFRAG_SERVICE_INTERVAL, 7200);
+ conf.setInt(OMConfigKeys.SNAPSHOT_DEFRAG_LIMIT_PER_TASK, 1);
+
+ omServiceId = "om-service-test-defrag";
+ cluster = MiniOzoneCluster.newHABuilder(conf)
+ .setOMServiceId(omServiceId)
+ .setNumOfOzoneManagers(3)
+ .build();
+
+ cluster.waitForClusterToBeReady();
+ client = cluster.newClient();
+ }
+
+ @AfterAll
+ public static void cleanup() {
+ IOUtils.closeQuietly(client);
+ if (cluster != null) {
+ cluster.shutdown();
+ }
+ }
+
+ /**
+ * Tests triggering snapshot defrag on the OM leader.
+ */
+ @Test
+ public void testDefragOnLeader() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ String leaderId = leader.getOMNodeId();
+
+ executeDefragCommand(leaderId, false);
+ }
+
+ /**
+ * Tests triggering snapshot defrag on an OM follower.
+ */
+ @Test
+ public void testDefragOnFollower() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ List<OzoneManager> allOMs = cluster.getOzoneManagersList();
+
+ // Find a follower OM
+ OzoneManager follower = null;
+ for (OzoneManager om : allOMs) {
+ if (!om.getOMNodeId().equals(leader.getOMNodeId())) {
+ follower = om;
+ break;
+ }
+ }
+
+ assertTrue(follower != null, "Should have at least one follower OM");
+ executeDefragCommand(follower.getOMNodeId(), false);
+ }
+
+ /**
+ * Tests triggering snapshot defrag on all OMs in the cluster.
+ */
+ @Test
+ public void testDefragOnAllOMs() throws Exception {
+ List<OzoneManager> allOMs = cluster.getOzoneManagersList();
+
+ assertEquals(3, allOMs.size(), "Expected 3 OMs in the cluster");
+
+ // Test defrag on each OM
+ for (OzoneManager om : allOMs) {
+ String omNodeId = om.getOMNodeId();
+ executeDefragCommand(omNodeId, false);
+ }
+ }
+
+ /**
+ * Tests triggering snapshot defrag with --no-wait option.
+ */
+ @Test
+ public void testDefragWithNoWait() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ String leaderId = leader.getOMNodeId();
+
+ executeDefragCommand(leaderId, true);
+ }
+
+ /**
+ * Tests triggering snapshot defrag on a follower with --no-wait option.
+ */
+ @Test
+ public void testDefragOnFollowerWithNoWait() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ List<OzoneManager> allOMs = cluster.getOzoneManagersList();
+
+ // Find a follower OM
+ OzoneManager follower = null;
+ for (OzoneManager om : allOMs) {
+ if (!om.getOMNodeId().equals(leader.getOMNodeId())) {
+ follower = om;
+ break;
+ }
+ }
+
+ assertTrue(follower != null, "Should have at least one follower OM");
Review Comment:
Use assertNotNull(follower, \"Should have at least one follower OM\")
instead of assertTrue(follower != null, ...). This provides better assertion
semantics and clearer failure messages in modern JUnit.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3573,6 +3573,48 @@ public boolean triggerRangerBGSync(boolean noWait)
throws IOException {
}
}
+ public boolean triggerSnapshotDefrag(boolean noWait) throws IOException {
+
+ // Note: Any OM (leader or follower) can run snapshot defrag
+
+ final UserGroupInformation ugi = getRemoteUser();
+ // Check Ozone admin privilege
+ if (!isAdmin(ugi)) {
+ throw new OMException("Only Ozone admins are allowed to trigger "
+ + "snapshot defragmentation manually", PERMISSION_DENIED);
+ }
+
+ // Get the SnapshotDefragService from KeyManager
+ final SnapshotDefragService defragService =
keyManager.getSnapshotDefragService();
+ if (defragService == null) {
+ throw new OMException("Snapshot defragmentation service is not
initialized",
+ FEATURE_NOT_ENABLED);
+ }
+
+ // Trigger Snapshot Defragmentation
+ if (noWait) {
+ final Thread t = new Thread(() -> {
+ try {
+ defragService.triggerSnapshotDefragOnce();
+ } catch (Exception e) {
+ LOG.error("Error during snapshot defragmentation", e);
+ }
+ }, threadPrefix + "SnapshotDefragTrigger-" + System.currentTimeMillis());
+ t.start();
+ LOG.info("User '{}' manually triggered Snapshot Defragmentation without
waiting"
+ + " in a new thread, tid = {}", ugi, t.getId());
+ return true;
+ } else {
+ LOG.info("User '{}' manually triggered Snapshot Defragmentation and is
waiting", ugi);
+ try {
+ return defragService.triggerSnapshotDefragOnce();
+ } catch (Exception e) {
+ LOG.error("Error during snapshot defragmentation", e);
+ return false;
+ }
Review Comment:
Exception handling in the synchronous path swallows all exceptions and
returns false, preventing the client from receiving meaningful error
information. This is inconsistent with the noWait=true path which allows
exceptions to propagate to the RPC layer (lines 3596-3602). Remove the
try-catch block here to allow exceptions to propagate to the caller, or at
least rethrow as IOException.
```suggestion
return defragService.triggerSnapshotDefragOnce();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3573,6 +3573,48 @@ public boolean triggerRangerBGSync(boolean noWait)
throws IOException {
}
}
+ public boolean triggerSnapshotDefrag(boolean noWait) throws IOException {
+
+ // Note: Any OM (leader or follower) can run snapshot defrag
+
+ final UserGroupInformation ugi = getRemoteUser();
+ // Check Ozone admin privilege
+ if (!isAdmin(ugi)) {
+ throw new OMException("Only Ozone admins are allowed to trigger "
+ + "snapshot defragmentation manually", PERMISSION_DENIED);
+ }
+
+ // Get the SnapshotDefragService from KeyManager
+ final SnapshotDefragService defragService =
keyManager.getSnapshotDefragService();
+ if (defragService == null) {
+ throw new OMException("Snapshot defragmentation service is not
initialized",
+ FEATURE_NOT_ENABLED);
+ }
+
+ // Trigger Snapshot Defragmentation
+ if (noWait) {
+ final Thread t = new Thread(() -> {
+ try {
+ defragService.triggerSnapshotDefragOnce();
+ } catch (Exception e) {
+ LOG.error("Error during snapshot defragmentation", e);
+ }
+ }, threadPrefix + "SnapshotDefragTrigger-" + System.currentTimeMillis());
Review Comment:
The thread created for async snapshot defragmentation is not managed
properly. Consider using an ExecutorService (similar to how other background
tasks are managed in OzoneManager) instead of creating raw threads. This would
provide better thread lifecycle management, error handling, and potential
thread pool reuse.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDefragAdmin.java:
##########
@@ -0,0 +1,219 @@
+/*
+ * 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.snapshot;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.util.List;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.utils.IOUtils;
+import org.apache.hadoop.ozone.MiniOzoneCluster;
+import org.apache.hadoop.ozone.MiniOzoneHAClusterImpl;
+import org.apache.hadoop.ozone.admin.OzoneAdmin;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.om.OMConfigKeys;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
+
+/**
+ * Integration test for 'ozone admin om snapshot defrag' command.
+ * Tests that the defrag command can be successfully triggered on any OM
+ * (leader or follower) in an HA cluster.
+ */
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public class TestSnapshotDefragAdmin {
+
+ private static MiniOzoneHAClusterImpl cluster;
+ private static OzoneClient client;
+ private static String omServiceId;
+
+ @BeforeAll
+ public static void init() throws Exception {
+ OzoneConfiguration conf = new OzoneConfiguration();
+ conf.setBoolean(OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY, true);
+ // Enable snapshot defrag service
+ conf.setInt(OMConfigKeys.OZONE_SNAPSHOT_DEFRAG_SERVICE_INTERVAL, 7200);
+ conf.setInt(OMConfigKeys.SNAPSHOT_DEFRAG_LIMIT_PER_TASK, 1);
+
+ omServiceId = "om-service-test-defrag";
+ cluster = MiniOzoneCluster.newHABuilder(conf)
+ .setOMServiceId(omServiceId)
+ .setNumOfOzoneManagers(3)
+ .build();
+
+ cluster.waitForClusterToBeReady();
+ client = cluster.newClient();
+ }
+
+ @AfterAll
+ public static void cleanup() {
+ IOUtils.closeQuietly(client);
+ if (cluster != null) {
+ cluster.shutdown();
+ }
+ }
+
+ /**
+ * Tests triggering snapshot defrag on the OM leader.
+ */
+ @Test
+ public void testDefragOnLeader() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ String leaderId = leader.getOMNodeId();
+
+ executeDefragCommand(leaderId, false);
+ }
+
+ /**
+ * Tests triggering snapshot defrag on an OM follower.
+ */
+ @Test
+ public void testDefragOnFollower() throws Exception {
+ OzoneManager leader = cluster.getOMLeader();
+ List<OzoneManager> allOMs = cluster.getOzoneManagersList();
+
+ // Find a follower OM
+ OzoneManager follower = null;
+ for (OzoneManager om : allOMs) {
+ if (!om.getOMNodeId().equals(leader.getOMNodeId())) {
+ follower = om;
+ break;
+ }
+ }
+
+ assertTrue(follower != null, "Should have at least one follower OM");
Review Comment:
Use assertNotNull(follower, \"Should have at least one follower OM\")
instead of assertTrue(follower != null, ...). This provides better assertion
semantics and clearer failure messages in modern JUnit.
--
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]
