peterxcli commented on code in PR #8124:
URL: https://github.com/apache/ozone/pull/8124#discussion_r2036382623
##########
hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/snapshot/ListSnapshotDiffHandler.java:
##########
@@ -37,38 +38,32 @@ public class ListSnapshotDiffHandler extends Handler {
@CommandLine.Mixin
private BucketUri snapshotPath;
- @CommandLine.Option(names = {"-s", "--status"},
+ @CommandLine.Option(names = {"--job-status"},
description = "List jobs based on status.\n" +
"Accepted values are: queued, in_progress, done, failed, rejected",
defaultValue = "in_progress")
private String jobStatus;
- @CommandLine.Option(names = {"-a", "--all"},
- description = "List all jobs regardless of status.",
- defaultValue = "false")
- private boolean listAll;
+ @CommandLine.Mixin
+ private ListOptions listOptions;
@Override
protected OzoneAddress getAddress() {
return snapshotPath.getValue();
}
@Override
- protected void execute(OzoneClient client, OzoneAddress address)
- throws IOException {
-
+ protected void execute(OzoneClient client, OzoneAddress address) throws
IOException {
String volumeName = snapshotPath.getValue().getVolumeName();
String bucketName = snapshotPath.getValue().getBucketName();
- List<OzoneSnapshotDiff> jobList =
- client.getObjectStore().listSnapshotDiffJobs(
- volumeName, bucketName, jobStatus, listAll);
+ Iterator<OzoneSnapshotDiff> iterator = client.getObjectStore()
+ .listSnapshotDiffJobs(volumeName, bucketName, jobStatus,
listOptions.isAll(), listOptions.getStartItem());
+
+ int counter = printAsJsonArray(iterator, listOptions.getLimit());
Review Comment:
Oh, got it — thanks for the clarification.
Btw, I just thought that maybe we could set `jobStatus` to an empty string
when `--all-status` is set. (need to throw some exception to user if
`--all-status` isn't set and jobStatus is also empty)
That way, the server-side logic could be:
```java
if (Strings.empty(jobStatus) || jobStatus == job.getStatus()) {
doAppendResponse();
}
```
And we could remove `listAllStatus` from the protobuf.
But `listAllStatus` does have more explicit logic, so please take this
suggestion with a grain of salt.
--
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]
