hemantk-12 commented on code in PR #4824: URL: https://github.com/apache/ozone/pull/4824#discussion_r1220984908
########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/snapshot/ListSnapshotDiffHandler.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.shell.snapshot; + +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob; +import org.apache.hadoop.ozone.shell.Handler; +import org.apache.hadoop.ozone.shell.OzoneAddress; +import org.apache.hadoop.ozone.shell.bucket.BucketUri; +import picocli.CommandLine; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; + +/** + * ozone sh snapshot listSnapshotDiff. + */ [email protected](name = "listSnapshotDiff", + description = "List snapshotDiff jobs for a bucket.") +public class ListSnapshotDiffHandler extends Handler { + + @CommandLine.Mixin + private BucketUri snapshotPath; + + @CommandLine.Option(names = {"-s, --status"}, + description = "List jobs based on status.\n" + + "Accepted values are: queued, in_progress, done, failed, rejected", + defaultValue = "in_progress") + private String jobStatus; + + @CommandLine.Option(names = {"-a, --all"}, + description = "List all jobs regardless of status.", + defaultValue = "false") + private boolean listAll; + + private static final String[] STATUS_VALUES = + {"queued", "in_progress", "done", "failed", "rejected"}; + + @Override + protected OzoneAddress getAddress() { + return snapshotPath.getValue(); + } + + @Override + protected void execute(OzoneClient client, OzoneAddress address) + throws IOException { + + String volumeName = snapshotPath.getValue().getVolumeName(); + String bucketName = snapshotPath.getValue().getBucketName(); + + if (Arrays.asList(STATUS_VALUES) Review Comment: Why not create `STATUS_VALUES` as a `List` itself? Do we really need this check? Whenever we add a new status, we have to add it here as well. ########## hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto: ########## @@ -1709,6 +1726,13 @@ message SnapshotDiffRequest { optional bool forceFullDiff = 7; } +message ListSnapshotDiffJobRequest { + optional string volumeName = 1; + optional string bucketName = 2; Review Comment: These two should be required. ########## hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/snapshot/SnapshotDiffResponse.java: ########## @@ -41,6 +41,15 @@ public JobStatusProto toProtobuf() { public static JobStatus fromProtobuf(JobStatusProto jobStatusProto) { return JobStatus.valueOf(jobStatusProto.name()); } + + public static JobStatus getJobStatusFromString(String jobStatus) { Review Comment: This helper function is not needed. Enum has [valueOf()](https://docs.oracle.com/javase/8/docs/api/java/lang/Enum.html#valueOf-java.lang.Class-java.lang.String-) which can be used directly. ``` JobStatus.valueOf(jobStatus); ``` You just have to capitalized String value of [jobStatus](https://github.com/apache/ozone/pull/4824/files#diff-46f82d5fbac5dc7aae1fc363811fb88e6bc6c89a2aefafddad90de2f1ff3cfbdR401) here. ########## hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java: ########## @@ -1072,6 +1073,20 @@ SnapshotDiffResponse snapshotDiff(String volumeName, String bucketName, boolean forceFullDiff) throws IOException; + /** + * Get a list of the SnapshotDiff jobs for a bucket based on the JobStatus. + * @param volumeName Name of the volume to which the snapshotted bucket belong + * @param bucketName Name of the bucket to which the snapshots belong + * @param jobStatus JobStatus to be used to filter the snapshot diff jobs Review Comment: Same as above. ########## hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto: ########## @@ -135,6 +135,7 @@ enum Type { SnapshotPurge = 118; RecoverLease = 119; SetTimes = 120; + ListSnapshotDiffJob = 121; Review Comment: nit: `ListSnapshotDiffJobs`. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java: ########## @@ -681,6 +686,14 @@ public SnapshotDiffResponse getSnapshotDiffReport(final String volume, return snapshotDiffReport; } + public List<SnapshotDiffJob> getSnapshotDiffList(final String volumeName, + final String bucketName, + final String jobStatus, + final boolean listAll) { + return snapshotDiffManager.getSnapshotDiffJobList( Review Comment: I think we should validate that `volumeName` and `volumeName` exist to fail fast? May also add snapshot existed check. ########## hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto: ########## @@ -789,6 +794,18 @@ message SnapshotInfo { optional int64 dbTxSequenceNumber = 12; } +message SnapshotDiffJobProto { + optional uint64 creationTime = 1; + optional string jobId = 2; Review Comment: I'm not sure if we should return internal parameters like `volumeName`, `jobId` and `totalDiffEntries`. ########## hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto: ########## @@ -1709,6 +1726,13 @@ message SnapshotDiffRequest { optional bool forceFullDiff = 7; } +message ListSnapshotDiffJobRequest { Review Comment: Should it be paginated? It doesn't have to be part of this PR. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java: ########## @@ -371,6 +378,35 @@ private Set<String> getSSTFileListForSnapshot(OmSnapshot snapshot, .getPath(), tablesToLookUp); } + public List<SnapshotDiffJob> getSnapshotDiffJobList( + String volumeName, String bucketName, + String jobStatus, boolean listAll) { + List<SnapshotDiffJob> jobList = new ArrayList<>(); + + try (ClosableIterator<Map.Entry<String, SnapshotDiffJob>> iterator = + snapDiffJobTable.iterator()) { + while (iterator.hasNext()) { + SnapshotDiffJob snapshotDiffJob = iterator.next().getValue(); + if (snapshotDiffJob.getVolume().equals(volumeName) && Review Comment: nit: to be null safe. ```suggestion if (Objects.equals(snapshotDiffJob.getVolume(), volumeName) && Objects.equals(snapshotDiffJob.getBucket(), bucketName)) { ``` ########## hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotDiffJob.java: ########## @@ -15,14 +15,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.hadoop.ozone.om.snapshot; +package org.apache.hadoop.ozone.om.helpers; Review Comment: Should we rename `SnapshotDiffJob` to `SnapshotDiffJobInfo` to be in alignment with other class? ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java: ########## @@ -371,6 +378,35 @@ private Set<String> getSSTFileListForSnapshot(OmSnapshot snapshot, .getPath(), tablesToLookUp); } + public List<SnapshotDiffJob> getSnapshotDiffJobList( + String volumeName, String bucketName, + String jobStatus, boolean listAll) { + List<SnapshotDiffJob> jobList = new ArrayList<>(); + + try (ClosableIterator<Map.Entry<String, SnapshotDiffJob>> iterator = + snapDiffJobTable.iterator()) { + while (iterator.hasNext()) { + SnapshotDiffJob snapshotDiffJob = iterator.next().getValue(); + if (snapshotDiffJob.getVolume().equals(volumeName) && + snapshotDiffJob.getBucket().equals(bucketName)) { + if (listAll) { + jobList.add(snapshotDiffJob); + continue; + } + + // If provided job status is invalid, + // then all jobs on the table will be ignored. + // No need to check if getJobStatusFromString doesn't return null. + if (snapshotDiffJob.getStatus().equals( Review Comment: It is better to use `==` for enum to be null safe. https://stackoverflow.com/a/1750453 ########## hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java: ########## @@ -0,0 +1,328 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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 org.apache.commons.lang3.RandomStringUtils; +import org.apache.hadoop.fs.FileUtil; +import org.apache.hadoop.hdds.client.StandaloneReplicationConfig; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.utils.db.cache.CacheKey; +import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.OmSnapshot; +import org.apache.hadoop.ozone.om.OmTestManagers; +import org.apache.hadoop.ozone.om.OzoneManager; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; +import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; +import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; +import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob; +import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; +import org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse; +import org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse.JobStatus; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.security.authentication.client.AuthenticationException; +import org.apache.ozone.test.GenericTestUtils; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.rocksdb.RocksDBException; + +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.UUID; +import java.util.concurrent.ThreadLocalRandom; + +import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor.ONE; +import static org.apache.hadoop.ozone.om.OmSnapshotManager.DELIMITER; + +/** + * Tests for {@link SnapshotDiffManager}. + */ +public class TestSnapshotDiffManager { + + private static final String VOLUME = "vol"; + private static final String BUCKET = "bucket"; + + private static File metaDir; + private static OzoneManager ozoneManager; + private static OMMetadataManager omMetadataManager; + private static SnapshotDiffManager snapshotDiffManager; + private static PersistentMap<String, SnapshotDiffJob> snapDiffJobTable; + + @BeforeAll + public static void init() throws AuthenticationException, + IOException, RocksDBException { + metaDir = GenericTestUtils.getRandomizedTestDir(); + if (!metaDir.exists()) { + Assertions.assertTrue(metaDir.mkdirs()); + } + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(OzoneConfigKeys.OZONE_METADATA_DIRS, + metaDir.getAbsolutePath()); + + OmTestManagers omTestManagers = new OmTestManagers(conf); + ozoneManager = omTestManagers.getOzoneManager(); + omMetadataManager = omTestManagers.getMetadataManager(); + + snapshotDiffManager = ozoneManager + .getOmSnapshotManager().getSnapshotDiffManager(); + snapDiffJobTable = snapshotDiffManager.getSnapDiffJobTable(); + + createVolumeAndBucket(); + } + + @AfterAll + public static void cleanUp() { + FileUtil.fullyDelete(metaDir); + } + + @Test + public void testListSnapshotDiffJobs() Review Comment: nit: It would be nicer if you convert this test to [parameterized test](https://www.baeldung.com/parameterized-tests-junit-5). Repetitive code can be avoid with that. Sample test: https://github.com/apache/ozone/blob/9034434285f3877246b3ebc6a7513c082931b279/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java#L263 ########## hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java: ########## @@ -611,4 +612,21 @@ public SnapshotDiffResponse snapshotDiff(String volumeName, return proxy.snapshotDiff(volumeName, bucketName, fromSnapshot, toSnapshot, token, pageSize, forceFullDiff); } + + /** + * Get a list of the SnapshotDiff jobs for a bucket based on the JobStatus. + * @param volumeName Name of the volume to which the snapshotted bucket belong + * @param bucketName Name of the bucket to which the snapshots belong + * @param jobStatus JobStatus to be used to filter the snapshot diff jobs Review Comment: Can you please add `listAll` too to this param list? -- 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]
