Skip to content

Conversation

@DaveTeng0
Copy link
Contributor

What changes were proposed in this pull request?

Command ozone admin namespace quota should return whole file system's disk capacity for root/volume/bucket.
(HDFS -df command returns whole file system's disk capacity for directory)

What is the link to the Apache JIRA

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

How was this patch tested?

manual test.

@DaveTeng0
Copy link
Contributor Author

cc. @jojochuang @umamaheswararao

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for a few import order issues.

Question: Can you help me understand why testQuotaUsage() had the expected quota output even before the fix?

import org.apache.hadoop.ozone.recon.spi.impl.OzoneManagerServiceProviderImpl;
import org.apache.hadoop.ozone.recon.spi.impl.StorageContainerServiceProviderImpl;
import org.apache.hadoop.ozone.recon.tasks.NSSummaryTaskWithFSO;
import org.apache.hadoop.ozone.recon.scm.ReconNodeManager;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this line up by one line to correct the import order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this one and the other ReconNodeManager import needs to move more than one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

import org.apache.hadoop.ozone.recon.spi.impl.OzoneManagerServiceProviderImpl;
import org.apache.hadoop.ozone.recon.spi.impl.StorageContainerServiceProviderImpl;
import org.apache.hadoop.ozone.recon.tasks.NSSummaryTaskWithLegacy;
import org.apache.hadoop.ozone.recon.scm.ReconNodeManager;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this line up by one line to correct the import order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@DaveTeng0
Copy link
Contributor Author

This PR has some dependency on changes made in another PR #3987 (HDDS-7472).

So, we could either merge 3987 first, or I do a cherry pick from 3987,
then the unit test failure in this PR should be quick to resolve after that! (I've done some tests on my local!)

@adoroszlai adoroszlai marked this pull request as draft November 23, 2022 07:20
@adoroszlai
Copy link
Contributor

This PR has some dependency on changes made in another PR

Then next time please open such PR as draft, to skip CI and let reviewers be aware of upcoming changes.

Or only open it after the other PR is merged.

@DaveTeng0
Copy link
Contributor Author

This PR has some dependency on changes made in another PR

Then next time please open such PR as draft, to skip CI and let reviewers be aware of upcoming changes.

Or only open it after the other PR is merged.

Yes! sure! thank you Atila!!

@DaveTeng0 DaveTeng0 marked this pull request as ready for review December 7, 2022 21:12
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make sure volume and bucket level quota does not change, only the root level quota should change.

@DaveTeng0
Copy link
Contributor Author

I think we need to make sure volume and bucket level quota does not change, only the root level quota should change.

yes, agree!

@jojochuang jojochuang merged commit 6064b84 into apache:master Dec 14, 2022
@jojochuang
Copy link
Contributor

Thanks @DaveTeng0 @adoroszlai

nishitpatira pushed a commit to nishitpatira/ozone that referenced this pull request Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants