You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/09/27 12:43:36 UTC
[GitHub] [ozone] chungen0126 opened a new pull request, #3784: HDDS-6854. Implement List Snapshot API
chungen0126 opened a new pull request, #3784:
URL: https://github.com/apache/ozone/pull/3784
## What changes were proposed in this pull request?
Implement list API which will list all the snapshots for a given Bucket.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6854
## How was this patch tested?
```
[INFO]
[INFO] --- maven-surefire-plugin:3.0.0-M5:test (default-test) @ ozone-manager ---
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.ozone.om.TestOmMetadataManager
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.734 s - in org.apache.hadoop.ozone.om.TestOmMetadataManager
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 51.848 s
[INFO] Finished at: 2022-09-27T18:21:19+08:00
[INFO] ------------------------------------------------------------------------
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r986015082
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,18 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket, filtered by a name prefix.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @param startKey the start snapshot name
Review Comment:
OK, I'll make this pr simple first.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019998291
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,15 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl merged pull request #3784: HDDS-6854. [Snapshot] Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3784:
URL: https://github.com/apache/ozone/pull/3784
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r994871377
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,100 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName,
+ String startKey, String prefix) throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> filteredSnapshotInfoMap = new TreeMap<>();
+ String seekStartKey;
+ boolean skipStartKey = false;
+ if (StringUtil.isNotBlank(startKey)) {
+ // Seek to the specified key.
+ seekStartKey = getOzoneKey(volumeName, bucketName, startKey);
+ skipStartKey = true;
+ } else {
+ // This allows us to seek directly to the first key with the right prefix.
+ seekStartKey = getOzoneKey(volumeName, bucketName,
+ StringUtil.isNotBlank(prefix) ? prefix : OM_KEY_PREFIX);
+ }
+
+ String seekPrefix;
+ if (StringUtil.isNotBlank(prefix)) {
+ seekPrefix = getOzoneKey(volumeName, bucketName, prefix);
+ } else {
+ seekPrefix = getBucketKey(volumeName, bucketName + OM_KEY_PREFIX);
+ }
+
+ filteredSnapshotInfoMap.putAll(
+ getSnapshotFromCache(seekStartKey, seekPrefix));
+ filteredSnapshotInfoMap.putAll(getSnapshotFromDB(seekStartKey, seekPrefix));
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : filteredSnapshotInfoMap
+ .entrySet()) {
+ if (cacheKey.getKey().equals(seekStartKey) && skipStartKey) {
+ continue;
+ }
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache(
+ String seekStartKey, String seekPrefix) {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>>> iterator =
+ snapshotInfoTable.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>> entry =
+ iterator.next();
+ String snapshotKey = entry.getKey().getCacheKey();
+ SnapshotInfo snapshotInfo = entry.getValue().getCacheValue();
+ if (snapshotInfo != null
+ && snapshotKey.startsWith(seekPrefix)
+ && snapshotKey.compareTo(seekStartKey) >= 0) {
+ result.put(snapshotKey, snapshotInfo);
+ }
+ }
+ return result;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromDB(
+ String seekStartKey, String seekPrefix) throws IOException {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ try (TableIterator<String, ? extends KeyValue<String, SnapshotInfo>>
+ snapshotIter = snapshotInfoTable.iterator()) {
+ KeyValue< String, SnapshotInfo> snapshotinfo;
+ snapshotIter.seek(seekStartKey);
+ while (snapshotIter.hasNext()) {
+ snapshotinfo = snapshotIter.next();
+ if (snapshotinfo != null && snapshotinfo.getKey()
+ .startsWith(seekPrefix)) {
+ CacheValue<SnapshotInfo> cacheValue =
+ snapshotInfoTable.getCacheValue(
+ new CacheKey<>(snapshotinfo.getKey()));
+ if (cacheValue == null || cacheValue.getCacheValue() != null) {
Review Comment:
I think that we don't need this condition. I have add some code comment for this as following.
There is always the latest data in the cache, so don't need to add earlier data from DB. We only add data from DB if there is no data in cache.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r994873238
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java:
##########
@@ -666,4 +668,66 @@ public void testAllTablesAreProperInOMMetadataManagerImpl() {
Assert.assertEquals(tablesByDefinition, tablesInManager);
}
+
+ @Test
+ public void testListSnapshot() throws Exception {
+ String vol1 = "vol1";
+ String bucket1 = "bucket1";
+
+ OMRequestTestUtils.addVolumeToDB(vol1, omMetadataManager);
+ addBucketsToCache(vol1, bucket1);
+ String prefixSnapshotA = "key-a";
+ String prefixSnapshotB = "key-b";
+ TreeSet<String> snapshotsASet = new TreeSet<>();
+ TreeSet<String> snapshotsSet = new TreeSet<>();
+
+ for (int i = 1; i <= 100; i++) {
+ if (i % 2 == 0) {
+ snapshotsASet.add(prefixSnapshotA + i);
+ snapshotsSet.add(prefixSnapshotA + i);
+ OMRequestTestUtils.addSnapshotToTable(vol1, bucket1,
+ prefixSnapshotA + i, omMetadataManager);
+ } else {
+ snapshotsSet.add(prefixSnapshotB + i);
+ OMRequestTestUtils.addSnapshotToTableCache(vol1, bucket1,
+ prefixSnapshotB + i, omMetadataManager);
+ }
+ }
+
+ //Test listing snapshots with no volume name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ null, null, null, null));
+
+ //Test listing snapshots with no bucket name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ vol1, null, null, null));
+
+ //Test listing all snapshots which have prefix "key-a".
+ List<SnapshotInfo> snapshotInfos = omMetadataManager.listSnapshot(vol1,
+ bucket1, null, prefixSnapshotA);
+ Assert.assertEquals(snapshotInfos.size(), 50);
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertTrue(snapshotInfo.getName().startsWith(prefixSnapshotA));
+ }
+
+ //Test listing snapshots which have prefix "key-a" and start with "key-a10".
+ String startKey = prefixSnapshotA + "10";
+ snapshotInfos = omMetadataManager.listSnapshot(
+ vol1, bucket1, startKey, prefixSnapshotA);
+ Assert.assertEquals(snapshotsASet.tailSet(startKey).size() - 1,
+ snapshotInfos.size());
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertFalse(snapshotInfo.getName().equals(startKey));
Review Comment:
This assertion doesn't need now.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1018608738
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
Review Comment:
> QQ: What's the purpose of OzoneSnapshot? or how is it different from SnapshotInfo? Is OzoneSnapshot for client while OzoneSnapshot for server?
SnapshotInfo is for communication between server, OzoneSnapshot is for client.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019422887
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ @Override
+ public List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(volumeName),
+ "volume can't be null or empty.");
+ Preconditions.checkArgument(Strings.isNotBlank(bucketName),
+ "bucket can't be null or empty.");
+ return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
+ .map(snapshotInfo -> new OzoneSnapshot(
+ snapshotInfo.getVolumeName(),
+ snapshotInfo.getBucketName(),
+ snapshotInfo.getName(),
+ snapshotInfo.getCreationTime(),
+ snapshotInfo.getSnapshotStatus(),
+ snapshotInfo.getSnapshotID(),
+ snapshotInfo.getSnapshotID())).collect(Collectors.toList());
Review Comment:
Also, shall we return `snapshotInfo.getCheckpointDir()` as well? Since `SnapshotID` doesn't really match the checkpoint dir suffix UUID.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019999291
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ @Override
+ public List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(volumeName),
+ "volume can't be null or empty.");
+ Preconditions.checkArgument(Strings.isNotBlank(bucketName),
+ "bucket can't be null or empty.");
+ return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
+ .map(snapshotInfo -> new OzoneSnapshot(
+ snapshotInfo.getVolumeName(),
+ snapshotInfo.getBucketName(),
+ snapshotInfo.getName(),
+ snapshotInfo.getCreationTime(),
+ snapshotInfo.getSnapshotStatus(),
+ snapshotInfo.getSnapshotID(),
+ snapshotInfo.getSnapshotID())).collect(Collectors.toList());
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
jojochuang commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r991465378
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ snapshotInfoMap.putAll(getSnapshotFromCache());
+ snapshotInfoMap.putAll(getSnapshotFromDB());
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache() {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
Review Comment:
this is fine. It would be even better if it passes snapshotInfoMap by reference, and add to snapshotInfoMap directly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3784:
URL: https://github.com/apache/ozone/pull/3784#issuecomment-1301469461
General comment. Would be great to add a robot test to test and verify the _shell_ list snapshot command by creating and listing snapshots.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019997997
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ @Override
+ public List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(volumeName),
+ "volume can't be null or empty.");
+ Preconditions.checkArgument(Strings.isNotBlank(bucketName),
+ "bucket can't be null or empty.");
+ return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r996771084
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -123,6 +123,7 @@ enum Type {
SetRangerServiceVersion = 107;
RangerBGSync = 109;
CreateSnapshot = 110;
+ ListSnapshot = 112;
Review Comment:
I fixed it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1012329902
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
Review Comment:
This client class does not appear to need to be extended from `WithMetadata`. None of its( `WithMetadata`) methods are used and the metadata datastructure within it is never used.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
Review Comment:
Small change. Using just `SnapshotInfo.SnapshotStatus `in this file. The import can _change to_ `import org.apache.hadoop.ozone.om.helpers.SnapshotInfo.SnapshotStatus`. Then declaration of variables can now be declared simply as` SnapshotStatus` type on lines ,
https://github.com/apache/ozone/blob/482f6d39c92e708200928490822a998995cee230/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java#L31
and
https://github.com/apache/ozone/blob/482f6d39c92e708200928490822a998995cee230/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java#L47
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,15 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
Review Comment:
rewording the return javadoc to `return list of snapshots for volume/bucket snapshot path` from _return list of snapshot name_.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
Review Comment:
rewording javadoc return to something like _return list of snapshots for volume/bucket snapshotpath_.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r982739755
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,100 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName,
+ String startKey, String prefix) throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> filteredSnapshotInfoMap = new TreeMap<>();
+ String seekStartKey;
+ boolean skipStartKey = false;
+ if (StringUtil.isNotBlank(startKey)) {
+ // Seek to the specified key.
+ seekStartKey = getOzoneKey(volumeName, bucketName, startKey);
+ skipStartKey = true;
+ } else {
+ // This allows us to seek directly to the first key with the right prefix.
+ seekStartKey = getOzoneKey(volumeName, bucketName,
+ StringUtil.isNotBlank(prefix) ? prefix : OM_KEY_PREFIX);
+ }
+
+ String seekPrefix;
+ if (StringUtil.isNotBlank(prefix)) {
+ seekPrefix = getOzoneKey(volumeName, bucketName, prefix);
+ } else {
+ seekPrefix = getBucketKey(volumeName, bucketName + OM_KEY_PREFIX);
+ }
+
+ filteredSnapshotInfoMap.putAll(
+ getSnapshotFromCache(seekStartKey, seekPrefix));
+ filteredSnapshotInfoMap.putAll(getSnapshotFromDB(seekStartKey, seekPrefix));
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : filteredSnapshotInfoMap
+ .entrySet()) {
+ if (cacheKey.getKey().equals(seekStartKey) && skipStartKey) {
+ continue;
+ }
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache(
+ String seekStartKey, String seekPrefix) {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>>> iterator =
+ snapshotInfoTable.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>> entry =
+ iterator.next();
+ String snapshotKey = entry.getKey().getCacheKey();
+ SnapshotInfo snapshotInfo = entry.getValue().getCacheValue();
+ if (snapshotInfo != null
+ && snapshotKey.startsWith(seekPrefix)
+ && snapshotKey.compareTo(seekStartKey) >= 0) {
+ result.put(snapshotKey, snapshotInfo);
+ }
+ }
+ return result;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromDB(
+ String seekStartKey, String seekPrefix) throws IOException {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ try (TableIterator<String, ? extends KeyValue<String, SnapshotInfo>>
+ snapshotIter = snapshotInfoTable.iterator()) {
+ KeyValue< String, SnapshotInfo> snapshotinfo;
+ snapshotIter.seek(seekStartKey);
+ while (snapshotIter.hasNext()) {
+ snapshotinfo = snapshotIter.next();
+ if (snapshotinfo != null && snapshotinfo.getKey()
+ .startsWith(seekPrefix)) {
+ CacheValue<SnapshotInfo> cacheValue =
+ snapshotInfoTable.getCacheValue(
+ new CacheKey<>(snapshotinfo.getKey()));
+ if (cacheValue == null || cacheValue.getCacheValue() != null) {
Review Comment:
I don't understand this:
```
cacheValue.getCacheValue() != null
```
should it be this?:
```
cacheValue.getCacheValue() == null
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019999022
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1157,6 +1157,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
Review Comment:
Sorry, I miss it while I take out those list option. I fixed it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1020005559
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1016231420
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java:
##########
@@ -1114,6 +1114,30 @@ public String createSnapshot(String volumeName,
return snapshotInfo.getName();
}
+ /**
+ * {@inheritDoc}
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1025882801
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetrics.java:
##########
@@ -430,6 +432,13 @@ public void incNumSnapshotCreateFails() {
numSnapshotCreateFails.incr();
}
+ public void incNumSnapshotLists() {
+ numSnapshotLists.incr();
+ }
+
+ public void incNumSnapshotListFails() {
+ numSnapshotListFails.incr();
+ }
Review Comment:
Not related to this jira. But this reminded me that we probably want to expose the number of active/inactive snapshots here as well at some point?
cc @neils-dev @GeorgeJahad
I filed HDDS-7506 to track this item.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
jojochuang commented on PR #3784:
URL: https://github.com/apache/ozone/pull/3784#issuecomment-1261312238
@chungen0126 thx for the PR. Please make sure the PR passes precommit tests in your local repo before posting to the Apache Ozone github :)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1018609558
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/snapshot/ListSnapshotHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.client.OzoneSnapshot;
+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.List;
+
+/**
+ * a handler for Ozone shell CLI command 'list snapshot'.
+ */
+@CommandLine.Command(name = "list",
+ aliases = "ls",
+ description = "list snapshot for the buckets.")
+public class ListSnapshotHandler extends Handler {
Review Comment:
I make this pr simple first.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r983985196
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,18 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket, filtered by a name prefix.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @param startKey the start snapshot name
Review Comment:
Whats the use case for using the startKey param ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r982773937
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java:
##########
@@ -666,4 +668,66 @@ public void testAllTablesAreProperInOMMetadataManagerImpl() {
Assert.assertEquals(tablesByDefinition, tablesInManager);
}
+
+ @Test
+ public void testListSnapshot() throws Exception {
+ String vol1 = "vol1";
+ String bucket1 = "bucket1";
+
+ OMRequestTestUtils.addVolumeToDB(vol1, omMetadataManager);
+ addBucketsToCache(vol1, bucket1);
+ String prefixSnapshotA = "key-a";
+ String prefixSnapshotB = "key-b";
+ TreeSet<String> snapshotsASet = new TreeSet<>();
+ TreeSet<String> snapshotsSet = new TreeSet<>();
+
+ for (int i = 1; i <= 100; i++) {
+ if (i % 2 == 0) {
+ snapshotsASet.add(prefixSnapshotA + i);
+ snapshotsSet.add(prefixSnapshotA + i);
+ OMRequestTestUtils.addSnapshotToTable(vol1, bucket1,
+ prefixSnapshotA + i, omMetadataManager);
+ } else {
+ snapshotsSet.add(prefixSnapshotB + i);
+ OMRequestTestUtils.addSnapshotToTableCache(vol1, bucket1,
+ prefixSnapshotB + i, omMetadataManager);
+ }
+ }
+
+ //Test listing snapshots with no volume name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ null, null, null, null));
+
+ //Test listing snapshots with no bucket name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ vol1, null, null, null));
+
+ //Test listing all snapshots which have prefix "key-a".
+ List<SnapshotInfo> snapshotInfos = omMetadataManager.listSnapshot(vol1,
+ bucket1, null, prefixSnapshotA);
+ Assert.assertEquals(snapshotInfos.size(), 50);
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertTrue(snapshotInfo.getName().startsWith(prefixSnapshotA));
+ }
+
+ //Test listing snapshots which have prefix "key-a" and start with "key-a10".
+ String startKey = prefixSnapshotA + "10";
+ snapshotInfos = omMetadataManager.listSnapshot(
+ vol1, bucket1, startKey, prefixSnapshotA);
+ Assert.assertEquals(snapshotsASet.tailSet(startKey).size() - 1,
+ snapshotInfos.size());
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertFalse(snapshotInfo.getName().equals(startKey));
Review Comment:
maybe this should be "greater than" instead of "not equals"?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019412285
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ @Override
+ public List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(volumeName),
+ "volume can't be null or empty.");
+ Preconditions.checkArgument(Strings.isNotBlank(bucketName),
+ "bucket can't be null or empty.");
+ return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
+ .map(snapshotInfo -> new OzoneSnapshot(
+ snapshotInfo.getVolumeName(),
+ snapshotInfo.getBucketName(),
+ snapshotInfo.getName(),
+ snapshotInfo.getCreationTime(),
+ snapshotInfo.getSnapshotStatus(),
+ snapshotInfo.getSnapshotID(),
+ snapshotInfo.getSnapshotID())).collect(Collectors.toList());
Review Comment:
Should be:
```suggestion
snapshotInfo.getSnapshotPath())).collect(Collectors.toList());
```
?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r996768683
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,137 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
+
+ /**
+ * Name of the Volume the snapshot belongs to.
Review Comment:
I fixed it.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java:
##########
@@ -268,6 +268,7 @@ public static boolean isReadOnly(
case TenantGetUserInfo:
case TenantListUser:
case RangerBGSync:
+ case ListSnapshot:
Review Comment:
I fixed it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
hemantk-12 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1014517372
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1157,6 +1157,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
Review Comment:
We are not using `volumeName` and `bucketName`. Don't we have to filter out response based on them?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ snapshotInfoMap.putAll(getSnapshotFromCache());
+ snapshotInfoMap.putAll(getSnapshotFromDB());
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache() {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>>> iterator =
+ snapshotInfoTable.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>> entry =
+ iterator.next();
+ String snapshotKey = entry.getKey().getCacheKey();
+ SnapshotInfo snapshotInfo = entry.getValue().getCacheValue();
+ if (snapshotInfo != null) {
+ result.put(snapshotKey, snapshotInfo);
+ }
+ }
+ return result;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromDB() throws IOException {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ try (TableIterator<String, ? extends KeyValue<String, SnapshotInfo>>
+ snapshotIter = snapshotInfoTable.iterator()) {
+ KeyValue< String, SnapshotInfo> snapshotinfo;
+ while (snapshotIter.hasNext()) {
+ snapshotinfo = snapshotIter.next();
+ if (snapshotinfo != null) {
+ CacheValue<SnapshotInfo> cacheValue =
+ snapshotInfoTable.getCacheValue(
+ new CacheKey<>(snapshotinfo.getKey()));
+ if (cacheValue == null) {
Review Comment:
1. Does cache get updated when new entries are added to DB?
2. What if key got evicted in between `appendSnapshotFromCacheToMap` and `appendSnapshotFromDBToMap`? Why not just all the entries from the DB? Would it be cleaner and better approach?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019995307
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1157,6 +1157,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ appendSnapshotFromCacheToMap(snapshotInfoMap);
+ appendSnapshotFromDBToMap(snapshotInfoMap);
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
jojochuang commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r991466154
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ snapshotInfoMap.putAll(getSnapshotFromCache());
+ snapshotInfoMap.putAll(getSnapshotFromDB());
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache() {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>>> iterator =
+ snapshotInfoTable.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>> entry =
+ iterator.next();
+ String snapshotKey = entry.getKey().getCacheKey();
+ SnapshotInfo snapshotInfo = entry.getValue().getCacheValue();
+ if (snapshotInfo != null) {
+ result.put(snapshotKey, snapshotInfo);
+ }
+ }
+ return result;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromDB() throws IOException {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ try (TableIterator<String, ? extends KeyValue<String, SnapshotInfo>>
+ snapshotIter = snapshotInfoTable.iterator()) {
+ KeyValue< String, SnapshotInfo> snapshotinfo;
+ while (snapshotIter.hasNext()) {
+ snapshotinfo = snapshotIter.next();
+ if (snapshotinfo != null) {
+ CacheValue<SnapshotInfo> cacheValue =
+ snapshotInfoTable.getCacheValue(
+ new CacheKey<>(snapshotinfo.getKey()));
+ if (cacheValue == null) {
Review Comment:
I don't really understand this part. Could you add code comments to explain why the snapshotinfo is only added when it's not in the cache?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r983990096
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,18 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket, filtered by a name prefix.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @param startKey the start snapshot name
Review Comment:
The API could be made rich or could be simple. The Base API could be simple here, returning all snapshots for a given bucket. You can add richness of parameters on top of that e.g. filtering capability based on name-pattern, name-range, creation-time range etc.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1022579914
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -968,5 +969,14 @@ Map<DatanodeDetails, OzoneInputStream>> getKeysEveryReplicas(
*/
String createSnapshot(String volumeName,
String bucketName, String snapshotName) throws IOException;
-
+
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
Review Comment:
Ok. But, I think this patch will be too large. I'll create another jira ticket to make this API paginated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019995549
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r996769001
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/snapshot/ListSnapshotHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.client.OzoneSnapshot;
+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.List;
+
+/**
+ * list ozone snapshot.
Review Comment:
I fixed it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r996767346
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1615,6 +1618,13 @@ message CreateSnapshotRequest {
optional string snapshotName = 3;
}
+message ListSnapshotRequest {
+ required string volumeName = 1;
+ required string bucketName = 2;
+ optional string startKey = 3;
Review Comment:
I'll make this pr simple first.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1018759710
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ snapshotInfoMap.putAll(getSnapshotFromCache());
+ snapshotInfoMap.putAll(getSnapshotFromDB());
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache() {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r983985196
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -548,5 +549,18 @@ public String createSnapshot(String volumeName,
return proxy.createSnapshot(volumeName, bucketName, snapshotName);
}
-
+ /**
+ * List snapshots in a volume/bucket, filtered by a name prefix.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @param startKey the start snapshot name
Review Comment:
Thank you @chungen0126 for working on this. Whats the use case for using the startKey param ? If we have StartKey, we should have end Key as well.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] prashantpogde commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
prashantpogde commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r983994179
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1615,6 +1618,13 @@ message CreateSnapshotRequest {
optional string snapshotName = 3;
}
+message ListSnapshotRequest {
+ required string volumeName = 1;
+ required string bucketName = 2;
+ optional string startKey = 3;
Review Comment:
If you plan to provide richer API, we should have more parameters here e.g. endKey, Start-Creation-time, End-creation time etc.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
jojochuang commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r991436478
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -123,6 +123,7 @@ enum Type {
SetRangerServiceVersion = 107;
RangerBGSync = 109;
CreateSnapshot = 110;
+ ListSnapshot = 112;
Review Comment:
why 112 not 111?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r994873238
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java:
##########
@@ -666,4 +668,66 @@ public void testAllTablesAreProperInOMMetadataManagerImpl() {
Assert.assertEquals(tablesByDefinition, tablesInManager);
}
+
+ @Test
+ public void testListSnapshot() throws Exception {
+ String vol1 = "vol1";
+ String bucket1 = "bucket1";
+
+ OMRequestTestUtils.addVolumeToDB(vol1, omMetadataManager);
+ addBucketsToCache(vol1, bucket1);
+ String prefixSnapshotA = "key-a";
+ String prefixSnapshotB = "key-b";
+ TreeSet<String> snapshotsASet = new TreeSet<>();
+ TreeSet<String> snapshotsSet = new TreeSet<>();
+
+ for (int i = 1; i <= 100; i++) {
+ if (i % 2 == 0) {
+ snapshotsASet.add(prefixSnapshotA + i);
+ snapshotsSet.add(prefixSnapshotA + i);
+ OMRequestTestUtils.addSnapshotToTable(vol1, bucket1,
+ prefixSnapshotA + i, omMetadataManager);
+ } else {
+ snapshotsSet.add(prefixSnapshotB + i);
+ OMRequestTestUtils.addSnapshotToTableCache(vol1, bucket1,
+ prefixSnapshotB + i, omMetadataManager);
+ }
+ }
+
+ //Test listing snapshots with no volume name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ null, null, null, null));
+
+ //Test listing snapshots with no bucket name.
+ Assert.assertThrows(OMException.class, () -> omMetadataManager.listSnapshot(
+ vol1, null, null, null));
+
+ //Test listing all snapshots which have prefix "key-a".
+ List<SnapshotInfo> snapshotInfos = omMetadataManager.listSnapshot(vol1,
+ bucket1, null, prefixSnapshotA);
+ Assert.assertEquals(snapshotInfos.size(), 50);
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertTrue(snapshotInfo.getName().startsWith(prefixSnapshotA));
+ }
+
+ //Test listing snapshots which have prefix "key-a" and start with "key-a10".
+ String startKey = prefixSnapshotA + "10";
+ snapshotInfos = omMetadataManager.listSnapshot(
+ vol1, bucket1, startKey, prefixSnapshotA);
+ Assert.assertEquals(snapshotsASet.tailSet(startKey).size() - 1,
+ snapshotInfos.size());
+ for (SnapshotInfo snapshotInfo : snapshotInfos) {
+ Assert.assertFalse(snapshotInfo.getName().equals(startKey));
Review Comment:
This assertion doen't need now.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on pull request #3784: HDDS-6854. [Snapshot] Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3784:
URL: https://github.com/apache/ozone/pull/3784#issuecomment-1319481800
Thanks @jojochuang @GeorgeJahad @prashantpogde @myskov @neils-dev @hemantk-12 for reviewing this.
Thanks @chungen0126 for the patch! I filed HDDS-7507 for pagination implementation.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
hemantk-12 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1010985671
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
Review Comment:
QQ: What's the purpose of `OzoneSnapshot`? or how is it different from `SnapshotInfo`? Is `OzoneSnapshot` for client while `OzoneSnapshot` for server?
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -931,6 +932,31 @@ public String createSnapshot(String volumeName,
bucketName, snapshotName);
}
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ @Override
+ public List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(volumeName),
+ "volume can't be null or empty.");
+ Preconditions.checkArgument(Strings.isNotBlank(bucketName),
+ "bucket can't be null or empty.");
+ return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
Review Comment:
May be create a function `toOzoneSnapshot` in `SnapshotInfo`.
```
return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
.map(snapshotInfo::toOzoneSnapshot)
.collect(Collectors.toList());
```
Or `fromSnapshotInfo` in `OzoneSnapshot`
```
return ozoneManagerClient.listSnapshot(volumeName, bucketName).stream()
.map(snapshotInfo -> OzoneSnapshot.fromSnapshotInfo(snapshotInfo))
.collect(Collectors.toList());
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -968,5 +969,14 @@ Map<DatanodeDetails, OzoneInputStream>> getKeysEveryReplicas(
*/
String createSnapshot(String volumeName,
String bucketName, String snapshotName) throws IOException;
-
+
+ /**
+ * List snapshots in a volume/bucket.
+ * @param volumeName volume name
+ * @param bucketName bucket name
+ * @return list of snapshot name
+ * @throws IOException
+ */
+ List<OzoneSnapshot> listSnapshot(String volumeName, String bucketName)
Review Comment:
Just curious why this API is not paginated?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1125,6 +1125,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ snapshotInfoMap.putAll(getSnapshotFromCache());
+ snapshotInfoMap.putAll(getSnapshotFromDB());
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromCache() {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>>> iterator =
+ snapshotInfoTable.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<SnapshotInfo>> entry =
+ iterator.next();
+ String snapshotKey = entry.getKey().getCacheKey();
+ SnapshotInfo snapshotInfo = entry.getValue().getCacheValue();
+ if (snapshotInfo != null) {
+ result.put(snapshotKey, snapshotInfo);
+ }
+ }
+ return result;
+ }
+
+ private TreeMap<String, SnapshotInfo> getSnapshotFromDB() throws IOException {
+ TreeMap<String, SnapshotInfo> result = new TreeMap<>();
+ try (TableIterator<String, ? extends KeyValue<String, SnapshotInfo>>
+ snapshotIter = snapshotInfoTable.iterator()) {
+ KeyValue< String, SnapshotInfo> snapshotinfo;
+ while (snapshotIter.hasNext()) {
+ snapshotinfo = snapshotIter.next();
+ if (snapshotinfo != null) {
+ CacheValue<SnapshotInfo> cacheValue =
+ snapshotInfoTable.getCacheValue(
+ new CacheKey<>(snapshotinfo.getKey()));
+ if (cacheValue == null) {
Review Comment:
+1
Do you mean that cache will fetch key from DB if it is cache miss?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1157,6 +1157,69 @@ public List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
return deletedKeys;
}
+ @Override
+ public List<SnapshotInfo> listSnapshot(String volumeName, String bucketName)
+ throws IOException {
+ if (Strings.isNullOrEmpty(volumeName)) {
+ throw new OMException("Volume name is required.",
+ ResultCodes.VOLUME_NOT_FOUND);
+ }
+
+ if (Strings.isNullOrEmpty(bucketName)) {
+ throw new OMException("Bucket name is required.",
+ ResultCodes.BUCKET_NOT_FOUND);
+ }
+
+ List<SnapshotInfo> snapshotInfos = new ArrayList<>();
+ TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
+
+ appendSnapshotFromCacheToMap(snapshotInfoMap);
+ appendSnapshotFromDBToMap(snapshotInfoMap);
+
+ for (Map.Entry<String, SnapshotInfo> cacheKey : snapshotInfoMap
+ .entrySet()) {
+ snapshotInfos.add(cacheKey.getValue());
+ }
+
+ return snapshotInfos;
Review Comment:
nit: cloud be simplify to:
```
TreeMap<String, SnapshotInfo> snapshotInfoMap = new TreeMap<>();
appendSnapshotFromCacheToMap(snapshotInfoMap);
appendSnapshotFromDBToMap(snapshotInfoMap);
return ArrayList<SnapshotInfo>(snapshotInfoMap.values());
```
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/snapshot/ListSnapshotHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.client.OzoneSnapshot;
+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.List;
+
+/**
+ * a handler for Ozone shell CLI command 'list snapshot'.
+ */
+@CommandLine.Command(name = "list",
+ aliases = "ls",
+ description = "list snapshot for the buckets.")
+public class ListSnapshotHandler extends Handler {
Review Comment:
Should we add listOptions similar to [List Bucket](https://github.com/hemantk-12/ozone/blob/23a0f743bc753048bbfa9ae7220bda198507b343/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/bucket/ListBucketHandler.java#L45) and [List Key](https://github.com/hemantk-12/ozone/blob/23a0f743bc753048bbfa9ae7220bda198507b343/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/ListKeyHandler.java#L45)?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1018608738
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+import org.apache.hadoop.ozone.om.helpers.WithMetadata;
+
+/**
+ * A class that encapsulates OzoneSnapshot.
+ */
+public class OzoneSnapshot extends WithMetadata {
Review Comment:
> QQ: What's the purpose of OzoneSnapshot? or how is it different from SnapshotInfo? Is OzoneSnapshot for client while OzoneSnapshot for server?
SnapshotInfo is for communication between server, OzoneSnapshot is for client.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] chungen0126 commented on a diff in pull request #3784: HDDS-6854. Implement List Snapshot API
Posted by GitBox <gi...@apache.org>.
chungen0126 commented on code in PR #3784:
URL: https://github.com/apache/ozone/pull/3784#discussion_r1019997588
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneSnapshot.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.client;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org