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 2019/12/21 10:14:51 UTC
[GitHub] [hadoop-ozone] cxorm opened a new pull request #385: HDDS-2779. Fix
list volume for --start parameter
cxorm opened a new pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385
## What changes were proposed in this pull request?
This PR was created to let ```--start``` work well in listing volume.
(In command ```ozone sh vol list --start=<startVolume>```)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-2779
## How was this patch tested?
1. UTs is updated, and ran.
2. Ran on standalone cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 merged pull request #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 merged pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-570779580
> > After fixing `listVolumes` with `startVolume`, the original `VolumeIterator#getNextListOfVolumes` would always have a non-empty list, and this situation leads to `VolumeIterator#hasNext` always be `true`.
> > So here we let the `VolumeIterator#getNextListOfVolumes` be empty list when there is no `volume` to be listed.
>
> The proper fix is to not return the `startVolume` as part of the result. If we do that, we don't need two different methods `getInitListOfVolumes`/`getNextListOfVolumes` for getting list of volumes.
Thanks @nandakumar131 for the review.
```ObjectStore``` change were reverted, and we exclude ```startVolume``` as part of listing result for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on issue #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-570555032
> After fixing `listVolumes` with `startVolume`, the original `VolumeIterator#getNextListOfVolumes` would always have a non-empty list, and this situation leads to `VolumeIterator#hasNext` always be `true`.
>
> So here we let the `VolumeIterator#getNextListOfVolumes` be empty list when there is no `volume` to be listed.
>
The proper fix is to not return the `startVolume` as part of the result. If we do that, we don't need two different methods `getInitListOfVolumes`/`getNextListOfVolumes` for getting list of volumes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-568689565
Sorry, there is something wrong when I test.
I'm going to fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we set `--start` with last volume.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we get empty list for the first round.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #385:
HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360789909
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
##########
@@ -299,11 +299,6 @@ public URI getKeyProviderUri() throws IOException {
@Override
public boolean hasNext() {
- if(!currentIterator.hasNext()) {
- currentIterator = getNextListOfVolumes(
- currentValue != null ? currentValue.getName() : null)
- .iterator();
- }
Review comment:
Thanks @nandakumar131 for this important info.
I deleted the block cause it would lead infinitely loop when I test ```listVolumes``` on my cluster.
I'm going to fix it. (without deleting 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on issue #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-568250705
Thanks @cxorm for working on this.
Suggestion:
You can simplify `listVolumes` code with the below one
```
public List<OmVolumeArgs> listVolumes(String userName,
String prefix, String startKey, int maxKeys) throws IOException {
if (StringUtil.isBlank(userName)) {
throw new OMException("User name is required to list Volumes.",
ResultCodes.USER_NOT_FOUND);
}
final List<OmVolumeArgs> result = Lists.newArrayList();
final List<String> volumes = getVolumesByUser(userName)
.getVolumeNamesList();
int index = 0;
if (!Strings.isNullOrEmpty(startKey)) {
index = volumes.indexOf(startKey.startsWith(OzoneConsts.OM_KEY_PREFIX) ?
startKey.substring(1) : startKey);
}
final String startChar = prefix == null ? "" : prefix;
while (index != -1 && index < volumes.size() && result.size() < maxKeys) {
final String volumeName = volumes.get(index);
if (volumeName.startsWith(startChar)) {
final OmVolumeArgs volumeArgs = getVolumeTable()
.get(getVolumeKey(volumeName));
if (volumeArgs == null) {
// Could not get volume info by given volume name,
// since the volume name is loaded from db,
// this probably means om db is corrupted or some entries are
// accidentally removed.
throw new OMException("Volume info not found for " + volumeName,
ResultCodes.VOLUME_NOT_FOUND);
}
result.add(volumeArgs);
}
index++;
}
return result;
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request
#385: HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360692843
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -815,34 +815,55 @@ public boolean isBucketEmpty(String volume, String bucket)
}
volumes = getVolumesByUser(userName);
- if (volumes == null || volumes.getVolumeNamesCount() == 0) {
+ if (volumes.getVolumeNamesCount() == 0) {
return result;
}
+ /* Process listing volumes with startVolume. */
boolean startKeyFound = Strings.isNullOrEmpty(startKey);
+ if (!startKeyFound) {
+ OmVolumeArgs startVolArgs = getVolumeTable()
+ .get(this.getVolumeKey(startKey));
+
+ if (startVolArgs == null) {
+ throw new OMException("StartVolume info not found for " + startKey,
+ ResultCodes.VOLUME_NOT_FOUND);
+ } else if (!startKey.startsWith(prefix)) {
Review comment:
We should also handle cases where the `startKey` doesn't match the given prefix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on issue #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-572640238
> > > I found that if we set `--start` with the last volume,
> > > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > > Would we fix it ? (or the behavior makes sense ?)
> >
> >
> > Thanks @cxorm for digging it. Yes, we should fix it.
> > If we set `--start` with the last volume, the result should be an empty list.
>
> Thanks @nandakumar131 for the comment.
>
> I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we get empty list in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L297).
>
> The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
>
> So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for it.)
Makes sense, thanks for fixing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-572883824
Thanks @nandakumar131 and @xiaoyuyao for the reviews.
Thanks @nandakumar131 for the merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571229457
Let's discuss the list --start behavior on slack.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-570779580
> > After fixing `listVolumes` with `startVolume`, the original `VolumeIterator#getNextListOfVolumes` would always have a non-empty list, and this situation leads to `VolumeIterator#hasNext` always be `true`.
> > So here we let the `VolumeIterator#getNextListOfVolumes` be empty list when there is no `volume` to be listed.
>
> The proper fix is to not return the `startVolume` as part of the result. If we do that, we don't need two different methods `getInitListOfVolumes`/`getNextListOfVolumes` for getting list of volumes.
Thanks @nandakumar131 for the review.
```ObjectStore``` change were reverted, and we exclude ```startVolume``` as part of listing result for now.
Sorry about a little question,
I found that if we set ```--start``` with the last volume,
the list will contain all volumes instead of empty result (like the ```--start``` behavior in listing buckets).
Would we fix it ? (or the behavior makes sense ?)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #385:
HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360874686
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
##########
@@ -299,11 +299,6 @@ public URI getKeyProviderUri() throws IOException {
@Override
public boolean hasNext() {
- if(!currentIterator.hasNext()) {
- currentIterator = getNextListOfVolumes(
- currentValue != null ? currentValue.getName() : null)
- .iterator();
- }
Review comment:
Fixed with checking existed volume.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on issue #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-572640926
Thanks @cxorm for the contribution and thanks @xiaoyuyao for the review. I will merge this shortly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on issue #385: HDDS-2779.
Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571076965
> I found that if we set `--start` with the last volume,
> the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> Would we fix it ? (or the behavior makes sense ?)
Thanks @cxorm for digging it. Yes, we should fix it.
If we set `--start` with the last volume, the result should be an empty list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we set `--start` with last volume and enter the second round.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we set `--start` with last volume.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request
#385: HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360690535
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
##########
@@ -299,11 +299,6 @@ public URI getKeyProviderUri() throws IOException {
@Override
public boolean hasNext() {
- if(!currentIterator.hasNext()) {
- currentIterator = getNextListOfVolumes(
- currentValue != null ? currentValue.getName() : null)
- .iterator();
- }
Review comment:
Without this logic, remote iteration will not work. Removing this will break the `listVolume` call if we try to list more than 1000 (`ozone.client.list.cache` ) volumes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request
#385: HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360691022
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -815,34 +815,55 @@ public boolean isBucketEmpty(String volume, String bucket)
}
volumes = getVolumesByUser(userName);
- if (volumes == null || volumes.getVolumeNamesCount() == 0) {
+ if (volumes.getVolumeNamesCount() == 0) {
return result;
}
+ /* Process listing volumes with startVolume. */
boolean startKeyFound = Strings.isNullOrEmpty(startKey);
+ if (!startKeyFound) {
+ OmVolumeArgs startVolArgs = getVolumeTable()
Review comment:
This will break the case where the user provides "/" at the start of volume name, we should also handle that case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we get empty list for the first time.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we get empty list in first round.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we set `--start` with last volume.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #385:
HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r360874686
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
##########
@@ -299,11 +299,6 @@ public URI getKeyProviderUri() throws IOException {
@Override
public boolean hasNext() {
- if(!currentIterator.hasNext()) {
- currentIterator = getNextListOfVolumes(
- currentValue != null ? currentValue.getName() : null)
- .iterator();
- }
Review comment:
Fixed with checking existed volume.
And work well in listing more than 1000 volumes with the same user.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request
#385: HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r362787945
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -807,33 +807,30 @@ public boolean isBucketEmpty(String volume, String bucket)
@Override
public List<OmVolumeArgs> listVolumes(String userName,
String prefix, String startKey, int maxKeys) throws IOException {
- List<OmVolumeArgs> result = Lists.newArrayList();
- UserVolumeInfo volumes;
+
if (StringUtil.isBlank(userName)) {
throw new OMException("User name is required to list Volumes.",
ResultCodes.USER_NOT_FOUND);
}
- volumes = getVolumesByUser(userName);
- if (volumes == null || volumes.getVolumeNamesCount() == 0) {
- return result;
- }
+ final List<OmVolumeArgs> result = Lists.newArrayList();
+ final List<String> volumes = getVolumesByUser(userName)
+ .getVolumeNamesList();
- boolean startKeyFound = Strings.isNullOrEmpty(startKey);
- for (String volumeName : volumes.getVolumeNamesList()) {
- if (!Strings.isNullOrEmpty(prefix)) {
- if (!volumeName.startsWith(prefix)) {
- continue;
- }
- }
+ int index = 0;
+ if (!Strings.isNullOrEmpty(startKey)) {
+ index = volumes.indexOf(
+ startKey.startsWith(OzoneConsts.OM_KEY_PREFIX) ?
+ startKey.substring(1) :
+ startKey);
Review comment:
Add `index = index != -1 ? index + 1 : index;` at line 826 to make sure that the `startKey` is not part of the returned result.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we set `--start` with last volume.
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was address this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm edited a comment on issue #385: HDDS-2779. Fix
list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-571139586
> > I found that if we set `--start` with the last volume,
> > the list will contain all volumes instead of empty result (like the `--start` behavior in listing buckets).
> > Would we fix it ? (or the behavior makes sense ?)
>
> Thanks @cxorm for digging it. Yes, we should fix it.
> If we set `--start` with the last volume, the result should be an empty list.
Thanks @nandakumar131 for the comment.
I think the `currentValue` in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L304) remaining `null` if and only if we get empty list in this [line](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java#L297).
The first round of `hasNext()` is called and enter the `if statement` (cause the empty-list), and then execute `currentIterator = getNextListOfVolumes(null).iterator();` that resulting in listing all volumes.
So fix was addressed this part. (The same as listing buckets, I'm going to create a Jira for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-572396076
Thanks @xiaoyuyao for the comment.
[Here](https://the-asf.slack.com/archives/C5RK7PWA1/p1576778714004100?thread_ts=1576771553.003500&cid=C5RK7PWA1) is the discussion about this issue in slack.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-568711796
After fixing ```listVolumes``` with ```startVolume```, the original ```VolumeIterator#getNextListOfVolumes``` would always have a non-empty list, and this situation leads to ```VolumeIterator#hasNext``` always be ```true```.
So here we let the ```VolumeIterator#getNextListOfVolumes``` be empty list when there is no ```volume``` to be listed.
Fixed, and ran on my standalone cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] nandakumar131 commented on a change in pull request
#385: HDDS-2779. Fix list volume for --start parameter
Posted by GitBox <gi...@apache.org>.
nandakumar131 commented on a change in pull request #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#discussion_r362788387
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -807,33 +807,30 @@ public boolean isBucketEmpty(String volume, String bucket)
@Override
public List<OmVolumeArgs> listVolumes(String userName,
String prefix, String startKey, int maxKeys) throws IOException {
- List<OmVolumeArgs> result = Lists.newArrayList();
- UserVolumeInfo volumes;
+
if (StringUtil.isBlank(userName)) {
throw new OMException("User name is required to list Volumes.",
ResultCodes.USER_NOT_FOUND);
}
- volumes = getVolumesByUser(userName);
- if (volumes == null || volumes.getVolumeNamesCount() == 0) {
- return result;
- }
+ final List<OmVolumeArgs> result = Lists.newArrayList();
+ final List<String> volumes = getVolumesByUser(userName)
+ .getVolumeNamesList();
- boolean startKeyFound = Strings.isNullOrEmpty(startKey);
- for (String volumeName : volumes.getVolumeNamesList()) {
- if (!Strings.isNullOrEmpty(prefix)) {
- if (!volumeName.startsWith(prefix)) {
- continue;
- }
- }
+ int index = 0;
+ if (!Strings.isNullOrEmpty(startKey)) {
+ index = volumes.indexOf(
+ startKey.startsWith(OzoneConsts.OM_KEY_PREFIX) ?
+ startKey.substring(1) :
+ startKey);
Review comment:
With this change, hopefully we don't need the changes made in `ObjectStore` class. Things should work as expected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on issue #385: HDDS-2779. Fix list
volume for --start parameter
Posted by GitBox <gi...@apache.org>.
cxorm commented on issue #385: HDDS-2779. Fix list volume for --start parameter
URL: https://github.com/apache/hadoop-ozone/pull/385#issuecomment-568457393
> Thanks @cxorm for working on this.
>
> Suggestion:
> You can simplify `listVolumes` code with the below one
>
> ```
> public List<OmVolumeArgs> listVolumes(String userName,
> String prefix, String startKey, int maxKeys) throws IOException {
>
> if (StringUtil.isBlank(userName)) {
> throw new OMException("User name is required to list Volumes.",
> ResultCodes.USER_NOT_FOUND);
> }
>
> final List<OmVolumeArgs> result = Lists.newArrayList();
> final List<String> volumes = getVolumesByUser(userName)
> .getVolumeNamesList();
>
> int index = 0;
> if (!Strings.isNullOrEmpty(startKey)) {
> index = volumes.indexOf(startKey.startsWith(OzoneConsts.OM_KEY_PREFIX) ?
> startKey.substring(1) : startKey);
> }
> final String startChar = prefix == null ? "" : prefix;
>
> while (index != -1 && index < volumes.size() && result.size() < maxKeys) {
> final String volumeName = volumes.get(index);
> if (volumeName.startsWith(startChar)) {
> final OmVolumeArgs volumeArgs = getVolumeTable()
> .get(getVolumeKey(volumeName));
> if (volumeArgs == null) {
> // Could not get volume info by given volume name,
> // since the volume name is loaded from db,
> // this probably means om db is corrupted or some entries are
> // accidentally removed.
> throw new OMException("Volume info not found for " + volumeName,
> ResultCodes.VOLUME_NOT_FOUND);
> }
> result.add(volumeArgs);
> }
> index++;
> }
> return result;
> }
> ```
Thanks @nandakumar131 for the suggestion.
Updated and ran the UT.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org