-
Notifications
You must be signed in to change notification settings - Fork 520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-3751. Ozone sh client support bucket quota option. #1412
Conversation
a89c0cc
to
8acd1b2
Compare
5327681
to
4a468a8
Compare
Hi @ChenSammi @cxorm, This PR is a bucket quota shell based on #1233 volume quota shell. Could you help review it? |
@@ -174,6 +184,20 @@ public OzoneBucket(ConfigurationSource conf, ClientProtocol proxy, | |||
this.modificationTime = Instant.ofEpochMilli(modificationTime); | |||
} | |||
|
|||
@SuppressWarnings("parameternumber") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: what is the purpose of @SuppressWarnings("parameternumber")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for @amaliujia‘s review.
Ozone's CheckStyle requires that the number of method variables must not exceed a certain value, or the check fails. So it borrows from other usage.
@@ -464,6 +469,8 @@ public void createBucket( | |||
.setStorageType(storageType) | |||
.setSourceVolume(bucketArgs.getSourceVolume()) | |||
.setSourceBucket(bucketArgs.getSourceBucket()) | |||
.setQuotaInBytes(quotaInBytes) | |||
.setQuotaInCounts(quotaInCounts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to verify whether quotaInBytes
and quotaInCounts
are valid? e.g. >= 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added a verify method In new commit.
} | ||
} | ||
|
||
private static void verifyQuota(long quotaInCounts, long quotaInBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This verifyQuota has the same function as above two. Suggest choose one and remove the other.
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/BucketArgs.java
Show resolved
Hide resolved
verifySpaceQuota(bucketArgs.getQuotaInBytes()); | ||
|
||
// When creating buckets using the API, if the user does not specify quota, | ||
// 0 is passed in by default, which should be set to -1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add default -1 value to newly added fields in proto file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In proto unsigned field can't have negative default value. The logic here is the same and I can encapsulate it as a method, which should be better
@Override | ||
public void setBucketQuota(String volumeName, String bucketName, | ||
long quotaInCounts, long quotaInBytes) throws IOException { | ||
HddsClientUtils.verifyResourceName(bucketName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verify volume
.setBucketName(bucketName) | ||
.setQuotaInBytes(quotaInBytes) | ||
.setQuotaInCounts(quotaInCounts); | ||
ozoneManagerClient.setBucketProperty(builder.build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setBucketQuota? setBucketProperty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bucket provides the setBucketProperty method and the corresponding request to modify the bucket information. This method already exists and is not something I added later. So here I'm reusing the method.
@@ -297,4 +301,28 @@ private BucketEncryptionInfoProto getBeinfo( | |||
CipherSuite.convert(metadata.getCipher()))); | |||
return bekb.build(); | |||
} | |||
|
|||
public void checkQuotaBytesValid(OmVolumeArgs omVolumeArgs, | |||
OmBucketInfo omBucketInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
description = "Quota in bytes of the newly created bucket (eg. 1GB)") | ||
private String quotaInBytes; | ||
|
||
@Option(names = {"--quota", "-q"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--quota -> --keyQuota -q -> -kq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest dropping the short option.
@Option(names = {"--quota", "-q"}, | |
@Option(names = {"--key-quota"}, |
description = "Quota in bytes of the newly created volume (eg. 1GB)") | ||
private String quotaInBytes; | ||
|
||
@Option(names = {"--quota", "-q"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, LGTM overall, left a couple of minor comment.
@@ -283,7 +285,8 @@ public static void addVolumeToDB(String volumeName, String ownerName, | |||
OmVolumeArgs omVolumeArgs = | |||
OmVolumeArgs.newBuilder().setCreationTime(Time.now()) | |||
.setVolume(volumeName).setAdminName(ownerName) | |||
.setOwnerName(ownerName).build(); | |||
.setOwnerName(ownerName).setQuotaInBytes(1024 * GB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to keep this QuotaInBytes
and QuotaInCounts
big enough, you can set it to Long.MAX_VALUE;
// The setting value cannot be greater than LONG.MAX_VALUE BYTES. | ||
try { | ||
store.getVolume(volumeName).getBucket(bucketName).setQuota( | ||
OzoneQuota.parseQuota("9999999999999GB", 100L)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use "9223372036854775808" here, it stand for the Long.MAX_VALUE + 1
/** | ||
* create bucket handler. | ||
*/ | ||
@Command(name = "update", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update -> setquota, could you please also change the volume quota update command to setquota?
We also need a remove quota CLI for both volume and bucket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @captainzmc for working on this.
@@ -46,6 +47,14 @@ | |||
"false/unspecified indicates otherwise") | |||
private Boolean isGdprEnforced; | |||
|
|||
@Option(names = {"--spaceQuota", "-sq"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short options should be only one character to support option grouping (ie. -sq
should be two separate options, same as -s -q
).
Long options should not be camel-case, rather lower-case using dash as separator.
@Option(names = {"--spaceQuota", "-sq"}, | |
@Option(names = {"--space-quota", "-s"}, |
description = "Quota in bytes of the newly created bucket (eg. 1GB)") | ||
private String quotaInBytes; | ||
|
||
@Option(names = {"--quota", "-q"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest dropping the short option.
@Option(names = {"--quota", "-q"}, | |
@Option(names = {"--key-quota"}, |
public class UpdateBucketHandler extends BucketHandler { | ||
|
||
@Option(names = {"--spaceQuota", "-sq"}, | ||
description = "Quota in bytes of the newly created volume (eg. 1GB)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider creating a mixin with the two quota options to ensure consistency and reduce duplication. See ListOptions
for example, and its usages in
and
Option descriptions can be generic, without mentioning "newly created volume" etc., so they can be applied to create|update volume|bucket.
import java.io.IOException; | ||
|
||
/** | ||
* create bucket handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: leftover doc
private long quotaInCounts = OzoneConsts.QUOTA_RESET; | ||
|
||
/** | ||
* Executes create bucket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: leftover doc
//Check quotaInBytes and quotaInCounts to update | ||
checkQuotaBytesValid(omVolumeArgs, omBucketInfo); | ||
checkQuotaCountsValid(omVolumeArgs, omBucketInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument validity should be checked before acquiring lock, preferably in preExecute
.
Also, please verify that the bucket is not a link, if quota is set in the request. Links cannot have actual content, so they should not have any quota defined, similar to encryption:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adoroszlai for the advice, the check action needs to get the volume or bucket in DB, so it better to do this action after acquiring the lock. And other comments has been fixed.
3967b86
to
2da6292
Compare
Thanks for @ChenSammi @adoroszlai and @maobaolong 's review. I had fixed review issues. Could you help take another look? |
@@ -66,17 +69,23 @@ | |||
* @param bucketEncryptionKey bucket encryption key name | |||
* @param sourceVolume | |||
* @param sourceBucket | |||
* @param quotaInBytes Volume quota in bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter statement is incorrent.
Iterator bucketIter = ozoneVolume.listBuckets(null); | ||
while (bucketIter.hasNext()) { | ||
OzoneBucket nextBucket = (OzoneBucket) bucketIter.next(); | ||
if(nextBucket.getQuotaInBytes() != QUOTA_RESET) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bucket has quota while Volume doesn't,this is a common case.
* Set Bucket Quota. | ||
* @param volumeName Name of the Volume. | ||
* @param bucketName Name of the Bucket. | ||
* @param quotaInBytes The maximum size this volume can be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param order doesn't match the real parmater order.
import picocli.CommandLine; | ||
|
||
/** | ||
* Common options for 'clrquota' comands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo comands
import java.io.IOException; | ||
|
||
/** | ||
* Executes update volume calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement is stale.
private SetSpaceQuotaOptions quotaOptions; | ||
|
||
@Option(names = {"--bucket-quota"}, | ||
description = "Bucket counts of the volume to set (eg. 5)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -> create
OmBucketInfo omBucketInfo) { | ||
long volumeQuotaInBytes = omVolumeArgs.getQuotaInBytes(); | ||
long quotaInBytes = omBucketInfo.getQuotaInBytes(); | ||
if(volumeQuotaInBytes < quotaInBytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need check sum of all bucket quota under the Volume. Better cover this case in UT.
We also need this check when update volume quato.
String volumeKey = omMetadataManager.getVolumeKey(volumeName); | ||
OmVolumeArgs omVolumeArgs = omMetadataManager.getVolumeTable() | ||
.get(volumeKey); | ||
if (checkQuotaBytesValid(omVolumeArgs, omBucketArgs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
af358b8
to
4d673fd
Compare
Thanks @captainzmc for the work. Would you please rebase this PR ? |
4d673fd
to
8f5c122
Compare
Hi all, status updates: rebase PR and resolve conflicts. This PR can be reviewed again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1.
@@ -314,6 +314,8 @@ enum Status { | |||
|
|||
PARTIAL_RENAME = 65; | |||
|
|||
BUCKET_SPACE_QUOTA_NOT_RESET = 66; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used anymore.
PARTIAL_RENAME | ||
PARTIAL_RENAME, | ||
|
||
BUCKET_SPACE_QUOTA_NOT_RESET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as other place.
Thanks @ChenSammi for the review. Review issues has been fixed. |
@@ -496,6 +491,28 @@ private static void verifyBucketName(String bucketName) throws OMException { | |||
} | |||
} | |||
|
|||
private static void verifyCountsQuota(long quota) throws OMException { | |||
if ((quota < OzoneConsts.QUOTA_RESET)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single ( is enough.
} | ||
|
||
private static void verifySpaceQuota(long quota) throws OMException { | ||
if ((quota < OzoneConsts.QUOTA_RESET)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
public void setVolumeQuota(String volumeName, long quotaInCounts, | ||
long quotaInBytes) throws IOException { | ||
public void setVolumeQuota(String volumeName, long quotaInBytes, | ||
long quotaInCounts) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we align the quotaInBytes and quotaInCounts order in setVolumeQuota and setBucketQuota? In another words, no need for the code change here.
private long quotaInCounts = OzoneConsts.QUOTA_RESET; | ||
|
||
/** | ||
* Executes create bucket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover statement
cca302c
to
d1b3636
Compare
d1b3636
to
81dba9e
Compare
LGTM, +1. Hi @adoroszlai,I'm not sure if you are satified with the change, would you like to take another look at this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @captainzmc for updating quota options. I think there is still room for improvement, but I'm fine with doing it in a follow-up, since this PR already has lots of review rounds.
builder.setQuotaInBytes(getQuotaValue(volArgs.getQuotaInBytes())); | ||
builder.setQuotaInCounts(getQuotaValue(volArgs.getQuotaInCounts())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguments are already checked, why change from use of the variables to getQuotaValue
?
builder.setQuotaInBytes(getQuotaValue(volArgs.getQuotaInBytes())); | |
builder.setQuotaInCounts(getQuotaValue(volArgs.getQuotaInCounts())); | |
builder.setQuotaInBytes(quotaInBytes); | |
builder.setQuotaInCounts(quotaInCounts); |
verifyCountsQuota(bucketArgs.getQuotaInCounts()); | ||
verifySpaceQuota(bucketArgs.getQuotaInBytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createBucket
verifies quota args, but createVolume
does not. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR of createVolume did not add a checkmark before, I will add this in the createVolume.
@CommandLine.Option(names = {"--key-quota"}, | ||
description = "clear count quota") | ||
private boolean clrKeyQuota; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using a bit more generic option name --count-quota
, this could be moved into ClearSpaceQuotaOptions
(and unified with volume's --bucket-quota
option).
@Option(names = {"--key-quota"}, | ||
description = "Key counts of the newly created bucket (eg. 5)") | ||
private long quotaInCounts = OzoneConsts.QUOTA_RESET; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to the option for ClearQuotaHandler
:
by using a bit more generic option name --count-quota
(and description), this could be moved into SetSpaceQuotaOptions
(and unified with volume's --bucket-quota
option).
Thanks @adoroszlai’s review, I have already fixed the review issues, can you help take another look? CI is normal in my personal branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @captainzmc for addressing my review comments.
Thanks @captainzmc for the contribution and @cxorm @adoroszlai @maobaolong for the review. |
What changes were proposed in this pull request?
By HDDS-3725 Ozone currently supports Set volume quota. This PR refers to the implementation of HDDS-3725, and make Ozone shell support set bucket quota.
The current Quota setting does not take effect. HDDS-541 gives all the work needed to perfect Quota.
This PR is a subtask of HDDS-541.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3751
How was this patch tested?
UT added