HDDS-2779. Fix list volume for --start parameter #385

cxorm · 2019-12-21T10:14:50Z

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?

UTs is updated, and ran.
Ran on standalone cluster.

nandakumar131 · 2019-12-22T08:39:16Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java

-      if(!currentIterator.hasNext()) {
-        currentIterator = getNextListOfVolumes(
-            currentValue != null ? currentValue.getName() : null)
-            .iterator();
-      }


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.

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.)

Fixed with checking existed volume.
And work well in listing more than 1000 volumes with the same user.

nandakumar131 · 2019-12-22T08:48:37Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

    boolean startKeyFound = Strings.isNullOrEmpty(startKey);
+    if (!startKeyFound) {
+      OmVolumeArgs startVolArgs = getVolumeTable()


This will break the case where the user provides "/" at the start of volume name, we should also handle that case.

nandakumar131 · 2019-12-22T09:21:48Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

+      if (startVolArgs == null) {
+        throw new OMException("StartVolume info not found for " + startKey,
+            ResultCodes.VOLUME_NOT_FOUND);
+      } else if (!startKey.startsWith(prefix)) {


We should also handle cases where the startKey doesn't match the given prefix.

nandakumar131 · 2019-12-22T10:42:44Z

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 reverts commit a40d61a.

cxorm · 2019-12-23T12:05:05Z

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.

cxorm · 2019-12-24T08:14:34Z

Sorry, there is something wrong when I test.
I'm going to fix it.

This reverts commit 1925ffb.

cxorm · 2019-12-24T09:57:49Z

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.

nandakumar131 · 2020-01-03T12:06:35Z

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.

nandakumar131 · 2020-01-03T12:08:07Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java

+      index = volumes.indexOf(
+          startKey.startsWith(OzoneConsts.OM_KEY_PREFIX) ?
+          startKey.substring(1) :
+          startKey);


Add index = index != -1 ? index + 1 : index; at line 826 to make sure that the startKey is not part of the returned result.

With this change, hopefully we don't need the changes made in ObjectStore class. Things should work as expected.

This reverts commit 7eda1a0.

This reverts commit 8691191.

cxorm · 2020-01-04T11:57:45Z

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 ?)

nandakumar131 · 2020-01-06T09:57:10Z

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.

cxorm · 2020-01-06T13:33:07Z

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 remaining null if and only if we get empty list in this line.

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.)

xiaoyuyao · 2020-01-06T17:24:59Z

Let's discuss the list --start behavior on slack.

cxorm · 2020-01-09T05:34:41Z

Thanks @xiaoyuyao for the comment.
Here is the discussion about this issue in slack.

nandakumar131 · 2020-01-09T16:29:53Z

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 remaining null if and only if we get empty list in this line.

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.

nandakumar131 · 2020-01-09T16:31:20Z

Thanks @cxorm for the contribution and thanks @xiaoyuyao for the review. I will merge this shortly.

cxorm · 2020-01-10T05:46:20Z

Thanks @nandakumar131 and @xiaoyuyao for the reviews.
Thanks @nandakumar131 for the merge.

cxorm added 2 commits December 21, 2019 18:06

Fix listing volumes with StartVolume

a40d61a

Update UTs

03bcf0c

nandakumar131 requested changes Dec 22, 2019

View reviewed changes

cxorm added 4 commits December 23, 2019 15:39

Revert "Fix listing volumes with StartVolume"

df0aeae

This reverts commit a40d61a.

Fix listing volumes with StartVolume

39dec1a

Fix UT

4e7a7f3

Fix VolumeIterator#hasNext

1925ffb

cxorm requested a review from nandakumar131 December 23, 2019 13:24

cxorm added 2 commits December 24, 2019 17:21

Revert "Fix VolumeIterator#hasNext"

4c1adfc

This reverts commit 1925ffb.

Fix VolumeIterator

7eda1a0

Update VolumeIterator#hasNext to handle startVol not found

8691191

nandakumar131 reviewed Jan 3, 2020

View reviewed changes

cxorm added 4 commits January 4, 2020 16:20

Revert "Fix VolumeIterator"

f7719ab

This reverts commit 7eda1a0.

Revert "Update VolumeIterator#hasNext to handle startVol not found"

bde3976

This reverts commit 8691191.

Exclude startVolume as part of list

f7be6cb

Update UT

1958f45

cxorm requested a review from nandakumar131 January 4, 2020 11:57

Update description of startVolume

33c52b3

cxorm added 2 commits January 6, 2020 20:48

Update description of startVolume

d5dac4e

List empty result when setting --start with last volume

76bd8c2

nandakumar131 approved these changes Jan 9, 2020

View reviewed changes

nandakumar131 merged commit 2fa37ef into apache:master Jan 9, 2020

lamberken mentioned this pull request Sep 16, 2020

HDDS-4234. Add important comment to ListVolumes logic #1417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-2779. Fix list volume for --start parameter #385

HDDS-2779. Fix list volume for --start parameter #385

cxorm commented Dec 21, 2019

nandakumar131 Dec 22, 2019

cxorm Dec 23, 2019

cxorm Dec 23, 2019 •

edited

Loading

nandakumar131 Dec 22, 2019

nandakumar131 Dec 22, 2019

nandakumar131 commented Dec 22, 2019

cxorm commented Dec 23, 2019

cxorm commented Dec 24, 2019

cxorm commented Dec 24, 2019

nandakumar131 commented Jan 3, 2020

nandakumar131 Jan 3, 2020

nandakumar131 Jan 3, 2020

cxorm commented Jan 4, 2020 •

edited

Loading

nandakumar131 commented Jan 6, 2020

cxorm commented Jan 6, 2020 •

edited

Loading

xiaoyuyao commented Jan 6, 2020

cxorm commented Jan 9, 2020

nandakumar131 commented Jan 9, 2020

nandakumar131 commented Jan 9, 2020

cxorm commented Jan 10, 2020

HDDS-2779. Fix list volume for --start parameter #385

HDDS-2779. Fix list volume for --start parameter #385

Conversation

cxorm commented Dec 21, 2019

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

nandakumar131 Dec 22, 2019

Choose a reason for hiding this comment

cxorm Dec 23, 2019

Choose a reason for hiding this comment

cxorm Dec 23, 2019 • edited Loading

Choose a reason for hiding this comment

nandakumar131 Dec 22, 2019

Choose a reason for hiding this comment

nandakumar131 Dec 22, 2019

Choose a reason for hiding this comment

nandakumar131 commented Dec 22, 2019

cxorm commented Dec 23, 2019

cxorm commented Dec 24, 2019

cxorm commented Dec 24, 2019

nandakumar131 commented Jan 3, 2020

nandakumar131 Jan 3, 2020

Choose a reason for hiding this comment

nandakumar131 Jan 3, 2020

Choose a reason for hiding this comment

cxorm commented Jan 4, 2020 • edited Loading

nandakumar131 commented Jan 6, 2020

cxorm commented Jan 6, 2020 • edited Loading

xiaoyuyao commented Jan 6, 2020

cxorm commented Jan 9, 2020

nandakumar131 commented Jan 9, 2020

nandakumar131 commented Jan 9, 2020

cxorm commented Jan 10, 2020

cxorm Dec 23, 2019 •

edited

Loading

cxorm commented Jan 4, 2020 •

edited

Loading

cxorm commented Jan 6, 2020 •

edited

Loading