HDDS-2610. Fix the ObjectStore#listVolumes failure when argument is null

[cxorm]

What changes were proposed in this pull request?

Fix the OmMetadataManagerImpl#listVolumes to list all volumes when the userName is null.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-2610

How was this patch tested?

UT Updating, and build & run on cluster.

[adoroszlai]

Thanks @cxorm for working on this.

[adoroszlai]

The result of this builder seems to be unused.

[adoroszlai]

Nit: Instead of creating a new builder in each iteration, a common builder could be created outside the loop, with common values (adminName) already initialized. Then in each iteration set the specific values and call build().

[adoroszlai]

I think this implementation could be slow if there are many users. Can the "all volumes" case be handled separately by iterating volumes directly from getVolumeTable? That would avoid: (1) iterating all users, (2) lookup of volumes for each user, (3) lookup of volume by volume name.

[bharatviswa504]

agreed. To list all Volumes this would be a better way to handle. And leave current code to handle listVolumes for a particular user.

[cxorm]

Thanks @bharatviswa504 for the review.
I handle the listAllVolumes separately, and updated.
UT ran well.

[bharatviswa504]

Thanks for catching this issue and fixing it.

With OM HA now we have Table Cache (And the same code is being used for OM non-HA). So, when the request is received we add it to the cache and then return the result. So, when we want to get all users in the system, we want to merge the data in User table cache and DB.

For a particular user list of Volumes is stored in UserVolumeInfo. So, there is no issue in that part.

For more info, you can refer to a recent fix that has been done in this part for listBuckets API (https://issues.apache.org/jira/browse/HDDS-1984). In a similar way, you need to change getAllUsers() to get all users from cache and DB. As now we want to support listAllVolumes in the system.

[bharatviswa504]

I feel @adoroszlai suggested way will be the good way to handle listAll volumes. And it will be easier, just iterate in-memory map of volume table cache.

[cxorm]

Thanks @bharatviswa504 for the review.
Fix in progress, hope soon.

[adoroszlai]

Thanks @cxorm for the update. Can you please further remove the unnecessary volumes list?

[cxorm]

Update listAllvolumes with parameter prefix and startKey for complete utility.

[adoroszlai]

Thanks @cxorm for continuing to improve the patch.

[cxorm]

Would you please take a look on the fix if you have time, @bharatviswa504

[arp7]

Hi @cxorm , looks like Attila has already approved these changes. Can you please rebase the patch, we can help commit it once rebased.

[cxorm]

The pr-check / unit (pull_request) seems not related to the patch.

[cxorm]

Sorry for letting me trigger it again.

[cxorm]

Thank you @arp7 for looking this PR.

The error check is not related to the patch.
Here is the same branch passed all checks.

[adoroszlai]

Filed HDDS-3151 for the integration test failure, which we've seen earlier without this change, too.

Acceptance test failure that SCM does not come out of safe mode is also observed elsewhere.

Given we have a clean run on the PR source branch, and it's only 2 commits behind master, I think it's safe to merge.

Thanks @cxorm for the contribution, @bharatviswa504 and @arp7 for the review.

[cxorm]

Thanks all for the help.