swamirishi commented on code in PR #6542:
URL: https://github.com/apache/ozone/pull/6542#discussion_r1568047728
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1345,30 +1345,30 @@ public List<SnapshotInfo> listSnapshot(
} else {
// This allows us to seek directly to the first key with the right
prefix.
seek = getOzoneKey(volumeName, bucketName,
- StringUtil.isNotBlank(
- snapshotPrefix) ? snapshotPrefix : OM_KEY_PREFIX);
+ StringUtil.isNotBlank(snapshotPrefix) ? snapshotPrefix :
OM_KEY_PREFIX);
}
List<SnapshotInfo> snapshotInfos = Lists.newArrayList();
+ String lastSnapshot = null;
try (ListIterator.MinHeapIterator snapshotIterator =
- new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
- bucketName, snapshotInfoTable)) {
- try {
- while (snapshotIterator.hasNext() && maxListResult > 0) {
- SnapshotInfo snapshotInfo =
- (SnapshotInfo) snapshotIterator.next().getValue();
- if (!snapshotInfo.getName().equals(prevSnapshot)) {
- snapshotInfos.add(snapshotInfo);
- maxListResult--;
- }
+ new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
bucketName, snapshotInfoTable)) {
+ SnapshotInfo snapshotInfo = null;
+ while (snapshotIterator.hasNext() && maxListResult > 0) {
+ snapshotInfo = (SnapshotInfo) snapshotIterator.next().getValue();
Review Comment:
```suggestion
SnapshotInfo snapshotInfo = (SnapshotInfo)
snapshotIterator.next().getValue();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1345,30 +1345,30 @@ public List<SnapshotInfo> listSnapshot(
} else {
// This allows us to seek directly to the first key with the right
prefix.
seek = getOzoneKey(volumeName, bucketName,
- StringUtil.isNotBlank(
- snapshotPrefix) ? snapshotPrefix : OM_KEY_PREFIX);
+ StringUtil.isNotBlank(snapshotPrefix) ? snapshotPrefix :
OM_KEY_PREFIX);
}
List<SnapshotInfo> snapshotInfos = Lists.newArrayList();
+ String lastSnapshot = null;
try (ListIterator.MinHeapIterator snapshotIterator =
- new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
- bucketName, snapshotInfoTable)) {
- try {
- while (snapshotIterator.hasNext() && maxListResult > 0) {
- SnapshotInfo snapshotInfo =
- (SnapshotInfo) snapshotIterator.next().getValue();
- if (!snapshotInfo.getName().equals(prevSnapshot)) {
- snapshotInfos.add(snapshotInfo);
- maxListResult--;
- }
+ new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
bucketName, snapshotInfoTable)) {
+ SnapshotInfo snapshotInfo = null;
+ while (snapshotIterator.hasNext() && maxListResult > 0) {
+ snapshotInfo = (SnapshotInfo) snapshotIterator.next().getValue();
+ if (!Objects.equals(snapshotInfo.getName(), prevSnapshot)) {
+ snapshotInfos.add(snapshotInfo);
+ maxListResult--;
}
- } catch (NoSuchElementException e) {
- throw new IOException(e);
- } catch (UncheckedIOException e) {
- throw e.getCause();
}
+ if (snapshotIterator.hasNext() && maxListResult == 0 && snapshotInfo !=
null) {
+ lastSnapshot = snapshotInfo.getName();
+ }
+ } catch (NoSuchElementException e) {
+ throw new IOException(e);
+ } catch (UncheckedIOException e) {
+ throw e.getCause();
}
- return snapshotInfos;
+ return new ListSnapshotResponse(snapshotInfos, lastSnapshot);
Review Comment:
Why not have a simple boolean here instead? The current logic already gets
the previous snapshotName which is at the end. Why have redundant string in the
response. We can have a boolean hasMore or something like this.
Correct me if I am wrong here, lastSnapshot and
snapshotInfos.get(snapshotInfos.size()-1).getName() will have the same value
right?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1345,30 +1345,30 @@ public List<SnapshotInfo> listSnapshot(
} else {
// This allows us to seek directly to the first key with the right
prefix.
seek = getOzoneKey(volumeName, bucketName,
- StringUtil.isNotBlank(
- snapshotPrefix) ? snapshotPrefix : OM_KEY_PREFIX);
+ StringUtil.isNotBlank(snapshotPrefix) ? snapshotPrefix :
OM_KEY_PREFIX);
}
List<SnapshotInfo> snapshotInfos = Lists.newArrayList();
+ String lastSnapshot = null;
try (ListIterator.MinHeapIterator snapshotIterator =
- new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
- bucketName, snapshotInfoTable)) {
- try {
- while (snapshotIterator.hasNext() && maxListResult > 0) {
- SnapshotInfo snapshotInfo =
- (SnapshotInfo) snapshotIterator.next().getValue();
- if (!snapshotInfo.getName().equals(prevSnapshot)) {
- snapshotInfos.add(snapshotInfo);
- maxListResult--;
- }
+ new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
bucketName, snapshotInfoTable)) {
+ SnapshotInfo snapshotInfo = null;
Review Comment:
```suggestion
boolean hasMore = false;
```
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/snapshot/ListSnapshotResponse.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.snapshot;
+
+import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
+
+import java.util.List;
+
+/**
+ * POJO for list snapshot info API.
+ */
+public class ListSnapshotResponse {
+ private final List<SnapshotInfo> snapshotInfos;
+ private final String lastSnapshot;
+
+ public ListSnapshotResponse(List<SnapshotInfo> snapshotInfos, String
lastSnapshot) {
Review Comment:
```suggestion
public ListSnapshotResponse(List<SnapshotInfo> snapshotInfos, boolean
hasMore) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1345,30 +1345,30 @@ public List<SnapshotInfo> listSnapshot(
} else {
// This allows us to seek directly to the first key with the right
prefix.
seek = getOzoneKey(volumeName, bucketName,
- StringUtil.isNotBlank(
- snapshotPrefix) ? snapshotPrefix : OM_KEY_PREFIX);
+ StringUtil.isNotBlank(snapshotPrefix) ? snapshotPrefix :
OM_KEY_PREFIX);
}
List<SnapshotInfo> snapshotInfos = Lists.newArrayList();
+ String lastSnapshot = null;
try (ListIterator.MinHeapIterator snapshotIterator =
- new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
- bucketName, snapshotInfoTable)) {
- try {
- while (snapshotIterator.hasNext() && maxListResult > 0) {
- SnapshotInfo snapshotInfo =
- (SnapshotInfo) snapshotIterator.next().getValue();
- if (!snapshotInfo.getName().equals(prevSnapshot)) {
- snapshotInfos.add(snapshotInfo);
- maxListResult--;
- }
+ new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
bucketName, snapshotInfoTable)) {
+ SnapshotInfo snapshotInfo = null;
+ while (snapshotIterator.hasNext() && maxListResult > 0) {
+ snapshotInfo = (SnapshotInfo) snapshotIterator.next().getValue();
+ if (!Objects.equals(snapshotInfo.getName(), prevSnapshot)) {
+ snapshotInfos.add(snapshotInfo);
+ maxListResult--;
}
- } catch (NoSuchElementException e) {
- throw new IOException(e);
- } catch (UncheckedIOException e) {
- throw e.getCause();
}
+ if (snapshotIterator.hasNext() && maxListResult == 0 && snapshotInfo !=
null) {
+ lastSnapshot = snapshotInfo.getName();
+ }
+ } catch (NoSuchElementException e) {
+ throw new IOException(e);
+ } catch (UncheckedIOException e) {
+ throw e.getCause();
}
- return snapshotInfos;
+ return new ListSnapshotResponse(snapshotInfos, lastSnapshot);
Review Comment:
```suggestion
return new ListSnapshotResponse(snapshotInfos, hasMore);
```
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1973,6 +1973,7 @@ message CreateSnapshotResponse {
message ListSnapshotResponse {
repeated SnapshotInfo snapshotInfo = 1;
+ optional string lastSnapshot = 2;
Review Comment:
```suggestion
optional boolean hasMore = 2;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1345,30 +1345,30 @@ public List<SnapshotInfo> listSnapshot(
} else {
// This allows us to seek directly to the first key with the right
prefix.
seek = getOzoneKey(volumeName, bucketName,
- StringUtil.isNotBlank(
- snapshotPrefix) ? snapshotPrefix : OM_KEY_PREFIX);
+ StringUtil.isNotBlank(snapshotPrefix) ? snapshotPrefix :
OM_KEY_PREFIX);
}
List<SnapshotInfo> snapshotInfos = Lists.newArrayList();
+ String lastSnapshot = null;
try (ListIterator.MinHeapIterator snapshotIterator =
- new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
- bucketName, snapshotInfoTable)) {
- try {
- while (snapshotIterator.hasNext() && maxListResult > 0) {
- SnapshotInfo snapshotInfo =
- (SnapshotInfo) snapshotIterator.next().getValue();
- if (!snapshotInfo.getName().equals(prevSnapshot)) {
- snapshotInfos.add(snapshotInfo);
- maxListResult--;
- }
+ new ListIterator.MinHeapIterator(this, prefix, seek, volumeName,
bucketName, snapshotInfoTable)) {
+ SnapshotInfo snapshotInfo = null;
+ while (snapshotIterator.hasNext() && maxListResult > 0) {
+ snapshotInfo = (SnapshotInfo) snapshotIterator.next().getValue();
+ if (!Objects.equals(snapshotInfo.getName(), prevSnapshot)) {
+ snapshotInfos.add(snapshotInfo);
+ maxListResult--;
}
- } catch (NoSuchElementException e) {
- throw new IOException(e);
- } catch (UncheckedIOException e) {
- throw e.getCause();
}
+ if (snapshotIterator.hasNext() && maxListResult == 0 && snapshotInfo !=
null) {
+ lastSnapshot = snapshotInfo.getName();
+ }
Review Comment:
```suggestion
hasMore = snapshotIterator.hasNext();
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -682,24 +663,31 @@ public boolean hasNext() {
@Override
public OzoneSnapshot next() {
if (hasNext()) {
- currentValue = currentIterator.next();
- return currentValue;
+ return currentIterator.next();
}
throw new NoSuchElementException();
}
- /**
- * Returns the next set of snapshot list using proxy.
- * @param prevSnapshot previous snapshot, this will be excluded from result
- * @return {@code List<OzoneSnapshot>}
- */
- private List<OzoneSnapshot> getNextListOfSnapshots(String prevSnapshot)
- throws IOException {
- return proxy.listSnapshot(volumeName, bucketName, snapshotPrefix,
- prevSnapshot, listCacheSize);
+ private void getNextListOfSnapshots(String startSnapshot) throws
IOException {
+ ListSnapshotResponse response =
+ proxy.listSnapshot(volumeName, bucketName, snapshotPrefix,
startSnapshot, listCacheSize);
+ currentIterator =
response.getSnapshotInfos().stream().map(OzoneSnapshot::fromSnapshotInfo).iterator();
+ lastSnapshot = response.getLastSnapshot();
Review Comment:
```suggestion
hasMore = response.hasMore();
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java:
##########
@@ -612,66 +614,45 @@ public OzoneSnapshot getSnapshotInfo(String volumeName,
* @param bucketName bucket name
* @param snapshotPrefix snapshot prefix to match
* @param prevSnapshot snapshots will be listed after this snapshot name
- * @return list of snapshots for volume/bucket snapshot path.
+ * @return an iterator of snapshots for volume/bucket snapshot path.
* @throws IOException
*/
- public Iterator<? extends OzoneSnapshot> listSnapshot(
- String volumeName, String bucketName, String snapshotPrefix,
- String prevSnapshot) throws IOException {
- return new SnapshotIterator(
- volumeName, bucketName, snapshotPrefix, prevSnapshot);
- }
-
- /**
- * Create an image of the current compaction log DAG in the OM.
- * @param fileNamePrefix file name prefix of the image file.
- * @param graphType type of node name to use in the graph image.
- * @return message which tells the image name, parent dir and OM leader
- * node information.
- */
- public String printCompactionLogDag(String fileNamePrefix,
- String graphType) throws IOException {
- return proxy.printCompactionLogDag(fileNamePrefix, graphType);
+ public Iterator<OzoneSnapshot> listSnapshot(String volumeName,
+ String bucketName,
+ String snapshotPrefix,
+ String prevSnapshot) throws
IOException {
+ return new SnapshotIterator(volumeName, bucketName, snapshotPrefix,
prevSnapshot);
}
/**
* An Iterator to iterate over {@link OzoneSnapshot} list.
*/
- private class SnapshotIterator implements Iterator<OzoneSnapshot> {
+ private final class SnapshotIterator implements Iterator<OzoneSnapshot> {
- private String volumeName = null;
- private String bucketName = null;
- private String snapshotPrefix = null;
+ private final String volumeName;
+ private final String bucketName;
+ private final String snapshotPrefix;
+ private String lastSnapshot = null;
Review Comment:
```suggestion
private boolean haseMore = true;
```
--
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]
