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 2020/07/22 02:38:01 UTC

[GitHub] [hadoop-ozone] captainzmc opened a new pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

captainzmc opened a new pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233


   ## What changes were proposed in this pull request?
   Currently, Ozone's support for Quota is incomplete, and currently only supports setting volume capacity Quota by command. The Quota setting based on quantity is not yet supported. The purpose of this PR is to support this.
   In addition, 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. Design documentation for Ozone Quota:
   https://docs.google.com/document/d/1ohbGn5N7FN6OD15xMShHH2SrtZRYx0-zUf9vjatn_OM/edit
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-3725
   
   ## How was this patch tested?
   
   Added 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482918376



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       This command follows the shell of volume (ozone sh **volume** update/create), so the volume in volumeQuota is redundant.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459222501



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/UpdateVolumeHandler.java
##########
@@ -39,22 +40,34 @@
       description = "Owner of the volume to set")
   private String ownerName;
 
-  @Option(names = {"--quota"},
-      description = "Quota of the volume to set"
-          + "(eg. 1G)")
-  private String quota;
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the volume to set (eg. 1GB)")
+  private String storagespaceQuota;
+
+  @Option(names = {"--quota", "-q"},
+      description = "Bucket counts of the volume to set (eg. 5)")
+  private long namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
 
   @Override
   protected void execute(OzoneClient client, OzoneAddress address)
       throws IOException {
-
     String volumeName = address.getVolumeName();
-
     OzoneVolume volume = client.getObjectStore().getVolume(volumeName);
-    if (quota != null && !quota.isEmpty()) {
-      volume.setQuota(OzoneQuota.parseQuota(quota));
+
+    long spaceQuota = volume.getStoragespaceQuota();
+    long countQuota = volume.getNamespaceQuota();
+
+    if (storagespaceQuota != null && !storagespaceQuota.isEmpty()) {
+      spaceQuota = OzoneQuota.parseQuota(storagespaceQuota,
+          namespaceQuota).getStoragespaceQuota();
+    }
+    if (namespaceQuota >= 0) {
+      countQuota = namespaceQuota;
     }
 
+    volume.setQuota(
+        OzoneQuota.getOzoneQuota(spaceQuota, countQuota));

Review comment:
       We could not violate `checkstyle` if we use one line here.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482918376



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       This command follows the shell of volume (ozone sh volume update/create), so use bucketQuota feels unclear, may be volume's quota of bucket number, or misinterpreted as bucket's quota. So, Here, the "quota" is named similar to HDFS, and more detailed description added.
   More detailed naming rules can be found in the [design documentation](https://issues.apache.org/jira/secure/attachment/13010452/Ozone%20Quota%20Design.pdf).




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



---------------------------------------------------------------------
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 pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662824611


   Thanks @captainzmc for the work.
   Overall the functionalily-part is good and some suggestions is added inline, test-related-part would be reviewd soon.
   
   Here I have two Questions IMHO.
   
   (1) Could we let the variable-pair be consistent ?
   I think it would better to use one of (`storagespaceQuota`/ `namespaceQuota`) and (`quotaInBytes`/ `quotaInCounts`).
   If it have other purpose to use both, please let me know.
   
   (2) Would we add limitation of creating volume and bucket in later patch ?
   I deployed this patch on my machine, and created volume without setting both of quota.
   I found that the storage of volume exceeds my local-storage.
   And I found we could create bucket even if the quota of bucket is -1.
   ![create_bucket](https://i.imgur.com/oN1Jf4X.png)


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459214380



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java
##########
@@ -149,6 +150,13 @@ public static String createStartupShutdownMessage(VersionInfo versionInfo,
         "  java = " + System.getProperty("java.version"));
   }
 
+  /**
+   * The same as String.format(Locale.ENGLISH, format, objects).
+   */
+  public static String format(final String format, final Object... objects) {
+    return String.format(Locale.ENGLISH, format, objects);
+  }
+

Review comment:
       I didn't found usage of this method in all places.
   Could you tell me why we add this part if I miss something ? 




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



---------------------------------------------------------------------
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 pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662872222


   Sorry for my careless huge "Add single comment" yesterday.
   
   There are Test-related-part and API-backward-compatibility-part to be reviewed. (If anyone continue reviewing after this comment.)


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r468557818



##########
File path: hadoop-ozone/dist/src/main/smoketest/topology/loaddata.robot
##########
@@ -25,7 +25,7 @@ Test Timeout        5 minutes
 
 *** Test Cases ***
 Create a volume, bucket and key
-    ${output} =         Execute          ozone sh volume create topvol1 --quota 100TB
+    ${output} =         Execute          ozone sh volume create topvol1

Review comment:
       This script is also used in the 0.5->0.6 upgrade. This script was created in the 0.5 environment and the 0.5 version does not support spaceQuota.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r483258119



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},

Review comment:
       I'm fine with the comment with @ChenSammi.
   Let's update 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458642861



##########
File path: hadoop-hdds/docs/content/shell/VolumeCommands.md
##########
@@ -37,12 +37,13 @@ assign it to a user.
 
 | Arguments                      |  Comment                                |
 |--------------------------------|-----------------------------------------|
-| -q, \-\-quota                    | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                    |
-| -u, \-\-user                     |  Required, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |
+| -sq, \-\-spaceQuota             | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                                        |
+| -q, \-\-quota                  | Optional, This argument that specifies the number of bucket in this volume can use in the Ozone cluster.                                        |

Review comment:
       ```suggestion
   | -q, \-\-quota                  | Optional, This argument that specifies the number of buckets in this volume can use in the Ozone 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-667990545


   Update PR, like HDDS-3991, Ignore protobuf lock files.
   Hi @cxorm. Previous issues has been solved. Could you help take another look？


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459196128



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {

Review comment:
       ```suggestion
       if (storagespaceQuota != null) {
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458723395



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,15 @@
       description = "Owner of of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-sq"},

Review comment:
       Do you mind change the "-sq" to "-s" (or other alphabet) ?
   
   I think the command-line promotion is better if it is alignment. 
   (And take this way, it would be left-alignment)
   
   ![out of alignment](https://i.imgur.com/nXhbsfW.png)
   ![left alignment](https://i.imgur.com/xzUAiZe.png)




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459468457



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Similar to [setOwner](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1697), there already exists [setQuota](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1727). If none of this is needed, I think we can separate PR to clean up the useless methods.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] codecov-commenter commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662279191


   # [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=h1) Report
   > Merging [#1233](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hadoop-ozone/commit/402a427f14f5b3d68956e93706de97d1ffae5b94&el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `68.25%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/graphs/tree.svg?width=650&height=150&src=pr&token=5YeeptJMby)](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1233      +/-   ##
   ============================================
   + Coverage     73.60%   73.64%   +0.03%     
   - Complexity    10070    10083      +13     
   ============================================
     Files           978      974       -4     
     Lines         49924    50010      +86     
     Branches       4853     4862       +9     
   ============================================
   + Hits          36749    36832      +83     
   - Misses        10847    10853       +6     
   + Partials       2328     2325       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [.../main/java/org/apache/hadoop/hdds/StringUtils.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy9TdHJpbmdVdGlscy5qYXZh) | `76.31% <0.00%> (-2.07%)` | `10.00 <0.00> (ø)` | |
   | [...main/java/org/apache/hadoop/ozone/OzoneConsts.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3Avb3pvbmUvT3pvbmVDb25zdHMuamF2YQ==) | `81.81% <0.00%> (-3.90%)` | `1.00 <0.00> (ø)` | |
   | [...hadoop/ozone/om/protocol/OzoneManagerProtocol.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL29tL3Byb3RvY29sL096b25lTWFuYWdlclByb3RvY29sLmphdmE=) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [.../java/org/apache/hadoop/ozone/om/OzoneManager.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9Pem9uZU1hbmFnZXIuamF2YQ==) | `67.72% <0.00%> (-0.05%)` | `202.00 <0.00> (ø)` | |
   | [.../org/apache/hadoop/ozone/om/VolumeManagerImpl.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9Wb2x1bWVNYW5hZ2VySW1wbC5qYXZh) | `13.52% <0.00%> (-0.05%)` | `7.00 <0.00> (ø)` | |
   | [...java/org/apache/hadoop/hdds/client/OzoneQuota.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy9jbGllbnQvT3pvbmVRdW90YS5qYXZh) | `53.01% <46.80%> (+13.32%)` | `10.00 <10.00> (+2.00)` | |
   | [...hadoop/ozone/shell/volume/UpdateVolumeHandler.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL3Rvb2xzL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3Avb3pvbmUvc2hlbGwvdm9sdW1lL1VwZGF0ZVZvbHVtZUhhbmRsZXIuamF2YQ==) | `70.00% <80.00%> (+11.66%)` | `2.00 <0.00> (ø)` | |
   | [.../org/apache/hadoop/ozone/client/rpc/RpcClient.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL2NsaWVudC9ycGMvUnBjQ2xpZW50LmphdmE=) | `86.35% <86.66%> (-0.20%)` | `78.00 <1.00> (ø)` | |
   | [...va/org/apache/hadoop/ozone/client/OzoneVolume.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL2NsaWVudC9Pem9uZVZvbHVtZS5qYXZh) | `84.44% <93.75%> (+0.91%)` | `21.00 <8.00> (+1.00)` | |
   | [...ava/org/apache/hadoop/ozone/client/VolumeArgs.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL2NsaWVudC9Wb2x1bWVBcmdzLmphdmE=) | `100.00% <100.00%> (ø)` | `8.00 <2.00> (+1.00)` | |
   | ... and [36 more](https://codecov.io/gh/apache/hadoop-ozone/pull/1233/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=footer). Last update [402a427...a2c5317](https://codecov.io/gh/apache/hadoop-ozone/pull/1233?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458651566



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -61,9 +61,13 @@
    */
   private String owner;
   /**
-   * Quota allocated for the Volume.
+   * Bytes Quota allocated for the Volume.
    */
-  private long quotaInBytes;
+  private long storagespaceQuota;
+  /**
+   * Bucket Counts Quota allocated for the Volume.

Review comment:
       Suggestion :
   "Quota of bucket count allocated for the 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459260086



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -204,6 +204,16 @@ public static Versioning getVersioning(boolean versioning) {
    */
   public static final long MAX_QUOTA_IN_BYTES = 1024L * 1024 * TB;
 
+  /**
+   * Quota value.
+   */
+  public static final long QUOTA_COUNT_RESET = -1;

Review comment:
       Could you please update the comment to "Quota of bucket counts" and add the description of meaning of "-1" here ?




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482913390



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},

Review comment:
       The abbreviation of this parameter was previously [confirmed with Yisheng](https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458723395). In general, the parameter abbreviation is a single letter, in order to keep the same with other parameters here sq->s.
   Of course, For better understanding, I added a detailed description. 




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



---------------------------------------------------------------------
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 pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm edited a comment on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-685179614


   Thank you @captainzmc for updating the PR. 
   Could you rebase it with latest master branch ?
   
   This PR looks good to me, +1 (Would you be so kind to take a look on this PR, @ChenSammi )


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459244192



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.

Review comment:
       ```suggestion
     // Quota to decide how many buckets can be created.
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459192304



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Instead of `OzoneManager.java`, this request is processed in `OMVolumeSetQuotaRequest.java` .
   
   We could not update code-snippet here.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459468457



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Similar to [setOwner](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1697), there already exists [setQuota](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1727). If none of this is needed, we can use separate PR to clean up this useless methods.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459200918



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {
+      volumeArgsBuilder.setStoragespaceQuota(storagespaceQuota);
     }
+
+    volumeArgsBuilder.setNamespaceQuotaQuota(namespaceQuota);

Review comment:
       The function name seems a little redundant.
   
   Suggestion is commented on `VolumeArgs.java`.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459244192



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.

Review comment:
       ```suggestion
     // Quota to decide how many buckets can be created.
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458643648



##########
File path: hadoop-hdds/docs/content/shell/VolumeCommands.md
##########
@@ -103,12 +104,13 @@ The volume update command allows changing of owner and quota on a given volume.
 
 | Arguments                      |  Comment                                |
 |--------------------------------|-----------------------------------------|
-| -q, \-\-quota                    | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                    |
-| -u, \-\-user                     |  Optional, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |
+| -sq, \-\-spaceQuota             | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                                        |
+| -q, \-\-quota                  | Optional, This argument that specifies the number of bucket in this volume can use in the Ozone cluster.                                        |

Review comment:
       ```suggestion
   | -q, \-\-quota                  | Optional, This argument that specifies the number of buckets in this volume can use in the Ozone 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc closed pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc closed pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233


   


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459245623



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.
+  private long namespaceQuota;
+  // Quota to decide how many storage space will be used in bytes.
+  private long storagespaceQuota;
+  private RawStorageSpaceQuota rawStoragespaceQuota;
+
+  private static class RawStorageSpaceQuota {

Review comment:
       The `RawStorageSpace` is a little confuse.
   Here we could add comment addressed to the class `RawStorageSpaceQuota` IMHO.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458675987



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -204,10 +215,18 @@ public String getOwner() {
    *
    * @return quotaInBytes
    */
-  public long getQuota() {
-    return quotaInBytes;
+  public long getStoragespaceQuota() {
+    return storagespaceQuota;
   }
 
+  /**
+   * Returns Bucket Quota count allocated for the Volume.

Review comment:
       Suggestion :
   "Returns quota of bucket count allocated for the 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459200918



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {
+      volumeArgsBuilder.setStoragespaceQuota(storagespaceQuota);
     }
+
+    volumeArgsBuilder.setNamespaceQuotaQuota(namespaceQuota);

Review comment:
       The function name seems a little redundant.
   
   Suggestion is commented on `VolumeArgs.java`




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r468441686



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,25 +135,28 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param quotaInBytesStr Volume quota in bytes String

Review comment:
       ```suggestion
      * @param quotaInBytes Volume quota in bytes
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,25 +135,28 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param quotaInBytesStr Volume quota in bytes String
+   * @param quotaInCounts Volume quota in counts
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String quotaInBytesStr,

Review comment:
       ```suggestion
     public static OzoneQuota parseQuota(String quotaInBytes,
   ```

##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -88,10 +88,12 @@
   /**
    * Changes the Quota on a volume.
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param quotaInCounts - Volume quota in counts.
+   * @param quotaInBytes - Volume quota in bytes.
    * @throws IOException
    */
-  void setQuota(String volume, long quota) throws IOException;
+  void setQuota(String volume, long quotaInCounts, long quotaInBytes)
+      throws IOException;

Review comment:
       The same as `ClientProtocol.java` part.
   For backward compatibility, I think we should add this method and also keep the original method.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -328,12 +332,17 @@ public boolean setVolumeOwner(String volumeName, String owner)
   }
 
   @Override
-  public void setVolumeQuota(String volumeName, OzoneQuota quota)
-      throws IOException {
-    verifyVolumeName(volumeName);
-    Preconditions.checkNotNull(quota);
-    long quotaInBytes = quota.sizeInBytes();
-    ozoneManagerClient.setQuota(volumeName, quotaInBytes);
+  public void setVolumeQuota(String volumeName, long quotaInCounts,

Review comment:
       As mention in interface side.

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -112,6 +168,13 @@ public static OzoneQuota parseQuota(String quotaString) {
       found = true;
     }
 
+    if (uppercase.endsWith(OZONE_QUOTA_KB)) {
+      size = uppercase
+          .substring(0, uppercase.length() - OZONE_QUOTA_KB.length());
+      currUnit = Units.KB;
+      found = true;
+    }
+

Review comment:
       IMHO I think this part could be added before encountering quota size of MB.
   
   And in line 200 to 201, we should add `KB` in 
   ```
   throw new IllegalArgumentException("Quota unit not recognized. " +
       "Supported values are BYTES, MB, GB and TB.");
   ```

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -101,10 +100,11 @@ void createVolume(String volumeName, VolumeArgs args)
   /**
    * Set Volume Quota.
    * @param volumeName Name of the Volume
-   * @param quota Quota to be set for the Volume
+   * @param quotaInBytes The maximum size this volume can be used.
+   * @param quotaInCounts The maximum number of buckets in this volume.
    * @throws IOException
    */
-  void setVolumeQuota(String volumeName, OzoneQuota quota)
+  void setVolumeQuota(String volumeName, long quotaInBytes, long quotaInCounts)

Review comment:
       For backward compatibility, I think we should add this method and also keep the original method.
   (User might implemented this interface and ran it on production.)
   
   @xiaoyuyao, would you please be so kind to provide some thoughts :smile: 

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -118,19 +124,19 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
   @SuppressWarnings("parameternumber")
   public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
       String name, String admin, String owner, long quotaInBytes,
-      long creationTime, long modificationTime, List<OzoneAcl> acls,
-      Map<String, String> metadata) {
-    this(conf, proxy, name, admin, owner, quotaInBytes,
+      long quotaInCounts, long creationTime, long modificationTime,
+      List<OzoneAcl> acls, Map<String, String> metadata) {
+    this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
         creationTime, acls, metadata);
     this.modificationTime = Instant.ofEpochMilli(modificationTime);
   }

Review comment:
       As mentioned above.

##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -258,14 +258,18 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param quotaInCounts - Volume quota in counts.
+   * @param quotaInBytes - Volume quota in bytes.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
+  public void setQuota(String volume, long quotaInCounts,
+      long quotaInBytes) throws IOException {
     SetVolumePropertyRequest.Builder req =
         SetVolumePropertyRequest.newBuilder();
-    req.setVolumeName(volume).setQuotaInBytes(quota);
+    req.setVolumeName(volume)
+       .setQuotaInBytes(quotaInBytes)
+       .setQuotaInCounts(quotaInCounts);

Review comment:
       As mentioned in `OzoneManagerProtocol.java`.

##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
##########
@@ -62,13 +64,15 @@
   @SuppressWarnings({"checkstyle:ParameterNumber", "This is invoked from a " +
       "builder."})
   private OmVolumeArgs(String adminName, String ownerName, String volume,
-                       long quotaInBytes, Map<String, String> metadata,
-                       OmOzoneAclMap aclMap, long creationTime,
-                       long modificationTime, long objectID, long updateID) {
+                       long quotaInBytes, long quotaInCounts,
+                       Map<String, String> metadata, OmOzoneAclMap aclMap,
+                       long creationTime, long modificationTime, long objectID,
+                       long updateID) {

Review comment:
       The same line could be indented by 4 space.

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1741,29 +1741,15 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param quotaInCounts - Volume quota in counts.
+   * @param quotaInBytes - Volume quota in bytes.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
-      checkAcls(ResourceType.VOLUME, StoreType.OZONE, ACLType.WRITE, volume,
-          null, null);
-    }
-
-    Map<String, String> auditMap = buildAuditMap(volume);
-    auditMap.put(OzoneConsts.QUOTA_IN_BYTES, String.valueOf(quota));
-    try {
-      metrics.incNumVolumeUpdates();
-      volumeManager.setQuota(volume, quota);
-      AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.SET_QUOTA,
-          auditMap));
-    } catch (Exception ex) {
-      metrics.incNumVolumeUpdateFails();
-      AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.SET_QUOTA,
-          auditMap, ex));
-      throw ex;
-    }
+  public void setQuota(String volume, long quotaInCounts,
+                       long quotaInBytes) throws IOException {
+    throw new UnsupportedOperationException("OzoneManager does not require " +
+        "this to be implemented. As this requests use a new approach");

Review comment:
       As mentioned in `OzoneManagerProtocol.java`.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -141,19 +147,23 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
   @SuppressWarnings("parameternumber")
   public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
       String name, String admin, String owner, long quotaInBytes,
-      long creationTime, long modificationTime, List<OzoneAcl> acls) {
-    this(conf, proxy, name, admin, owner, quotaInBytes, creationTime, acls);
+      long quotaInCounts, long creationTime, long modificationTime,
+      List<OzoneAcl> acls) {
+    this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
+        creationTime, acls);
     this.modificationTime = Instant.ofEpochMilli(modificationTime);
   }

Review comment:
       As mentioned above.

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},
-      description =
-          "Quota of the newly created volume (eg. 1G)")
-  private String quota;
+      description = "Bucket counts of the newly created volume (eg. 5)")
+  private long quotaInCounts = OzoneConsts.QUOTA_COUNT_RESET;

Review comment:
       Some discussion on naming parameters is updated in design-doc.
   
   We could fix/update here after discussion. 

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -94,13 +98,15 @@
   @SuppressWarnings("parameternumber")
   public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
       String name, String admin, String owner, long quotaInBytes,
-      long creationTime, List<OzoneAcl> acls, Map<String, String> metadata) {
+      long quotaInCounts, long creationTime, List<OzoneAcl> acls,
+      Map<String, String> metadata) {
     Preconditions.checkNotNull(proxy, "Client proxy is not set.");
     this.proxy = proxy;
     this.name = name;
     this.admin = admin;
     this.owner = owner;
     this.quotaInBytes = quotaInBytes;
+    this.quotaInCounts = quotaInCounts;
     this.creationTime = Instant.ofEpochMilli(creationTime);
     this.acls = acls;
     this.listCacheSize = HddsClientUtils.getListCacheSize(conf);

Review comment:
       For backward compatibility, I think we should add new API for new attribute and also keep the original public method.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -118,19 +124,19 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
   @SuppressWarnings("parameternumber")
   public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
       String name, String admin, String owner, long quotaInBytes,
-      long creationTime, long modificationTime, List<OzoneAcl> acls,
-      Map<String, String> metadata) {
-    this(conf, proxy, name, admin, owner, quotaInBytes,
+      long quotaInCounts, long creationTime, long modificationTime,
+      List<OzoneAcl> acls, Map<String, String> metadata) {
+    this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
         creationTime, acls, metadata);
     this.modificationTime = Instant.ofEpochMilli(modificationTime);
   }
 
   @SuppressWarnings("parameternumber")
   public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
       String name, String admin, String owner, long quotaInBytes,
-      long creationTime, List<OzoneAcl> acls) {
-    this(conf, proxy, name, admin, owner, quotaInBytes, creationTime, acls,
-        new HashMap<>());
+      long quotaInCounts, long creationTime, List<OzoneAcl> acls) {
+    this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
+        creationTime, acls, new HashMap<>());

Review comment:
       As mentioned above.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458642279



##########
File path: hadoop-hdds/docs/content/shell/VolumeCommands.md
##########
@@ -37,12 +37,13 @@ assign it to a user.
 
 | Arguments                      |  Comment                                |
 |--------------------------------|-----------------------------------------|
-| -q, \-\-quota                    | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                    |
-| -u, \-\-user                     |  Required, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |
+| -sq, \-\-spaceQuota             | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                                        |
+| -q, \-\-quota                  | Optional, This argument that specifies the number of bucket in this volume can use in the Ozone cluster.                                        |
+| -u, \-\-user                   | Required, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |

Review comment:
       ```suggestion
   | -u, \-\-user                   | Required, The name of the user who owns this volume. This user can create buckets and keys on this 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] ChenSammi commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-687007534


   LGTM +1.  
   
   Thanks @captainzmc for the contribution and @cxorm for review the patch. 


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482918376



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       This command follows the shell of volume (Ozone sh Volume update/create), so the volume in volumeQuota is redundant.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc edited a comment on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc edited a comment on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-682285487


   Had fixed issues as HDDS-4129 suggested. Use Long.MAX_VALUE to determine if the parameter values for quota are reasonable. CC @ChenSammi.


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r483384591



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},

Review comment:
       Agreed, I will fix the naming problem.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459249493



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -53,26 +102,28 @@ public long getSize() {
    * @return Unit in MB, GB or TB
    */
   public Units getUnit() {
-    return unit;
+    return this.rawStoragespaceQuota.getUnit();
   }
 
   /**
    * Constructs a default Quota object.
    */
-  public OzoneQuota() {
-    this.size = 0;
-    this.unit = Units.UNDEFINED;
+  private OzoneQuota() {
+    this.namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
+    this.storagespaceQuota = OzoneConsts.MAX_QUOTA_IN_BYTES;
   }
 
   /**
    * Constructor for Ozone Quota.
    *
-   * @param size Long Size
-   * @param unit MB, GB  or TB
+   * @param namespaceQuota long value
+   * @param rawStoragespaceQuota RawStorageSpaceQuota value
    */
-  public OzoneQuota(long size, Units unit) {
-    this.size = size;
-    this.unit = unit;
+  private OzoneQuota(long namespaceQuota,
+                     RawStorageSpaceQuota rawStoragespaceQuota) {

Review comment:
       ```suggestion
     private OzoneQuota(long namespaceQuota,
         RawStorageSpaceQuota rawStoragespaceQuota) {
   ```




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



---------------------------------------------------------------------
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 removed a comment on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm removed a comment on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662824611


   Thanks @captainzmc for the work.
   Overall the functionalily-part is good and some suggestions is added inline, test-related-part would be reviewd soon.
   
   Here I have two Questions IMHO.
   
   (1) Could we let the variable-pair be consistent ?
   I think it would better to use one of (`storagespaceQuota`/ `namespaceQuota`) and (`quotaInBytes`/ `quotaInCounts`).
   If it have other purpose to use both, please let me know.
   
   (2) Would we add limitation of creating volume and bucket in later patch ?
   I deployed this patch on my machine, and created volume without setting both of quota.
   I found that the storage of volume exceeds my local-storage.
   And I found we could create bucket even if the quota of bucket is -1.
   ![create_bucket](https://i.imgur.com/oN1Jf4X.png)


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459192304



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Instead of `OzoneManager.java`, this request is processed in `OMVolumeSetQuotaRequest.java` .
   
   We could not update code-snippet here.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459245623



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.
+  private long namespaceQuota;
+  // Quota to decide how many storage space will be used in bytes.
+  private long storagespaceQuota;
+  private RawStorageSpaceQuota rawStoragespaceQuota;
+
+  private static class RawStorageSpaceQuota {

Review comment:
       The `RawStorageSpace` is a little confuse.
   Here we could add comment addressed to the class `RawStorageSpaceQuota` IMHO.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459194918



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManagerImpl.java
##########
@@ -273,12 +272,14 @@ private void setOwnerCommitToDB(UserVolumeInfo oldOwnerVolumeList,
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in count for bucket.
+   * @param storagespaceQuota - Quota in bytes.
    *
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
+  public void setQuota(String volume, long namespaceQuota,
+                       long storagespaceQuota) throws IOException {

Review comment:
       This part is the same as comments on `OzoneManager.java`
   
   




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r468557818



##########
File path: hadoop-ozone/dist/src/main/smoketest/topology/loaddata.robot
##########
@@ -25,7 +25,7 @@ Test Timeout        5 minutes
 
 *** Test Cases ***
 Create a volume, bucket and key
-    ${output} =         Execute          ozone sh volume create topvol1 --quota 100TB
+    ${output} =         Execute          ozone sh volume create topvol1

Review comment:
       This script is also used in the 0.5->0.6 upgrade. This script was used in the 0.5 environment and the 0.5 version does not support -spaceQuota.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459200596



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -122,8 +136,13 @@ public String getQuota() {
       return this;
     }
 
-    public VolumeArgs.Builder setQuota(String quota) {
-      this.volumeQuota = quota;
+    public VolumeArgs.Builder setStoragespaceQuota(String quota) {
+      this.storagespaceQuota = quota;
+      return this;
+    }
+
+    public VolumeArgs.Builder setNamespaceQuotaQuota(long quota) {

Review comment:
       ```suggestion
       public VolumeArgs.Builder setNamespaceQuota(long quota) {
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459202326



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -33,25 +33,30 @@
 
   private final String admin;
   private final String owner;
-  private final String quota;
+  private final String storagespaceQuota;
+  private final long namespaceQuota;
   private final List<OzoneAcl> acls;
   private Map<String, String> metadata;
 
   /**
    * Private constructor, constructed via builder.
    * @param admin Administrator's name.
    * @param owner Volume owner's name
-   * @param quota Volume Quota.
+   * @param storagespaceQuota Volume Quota.
+   * @param namespaceQuota Volume Quota.

Review comment:
       I think we could update the description of the parameter consistent with shell-command IMHO.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-686931995


   Thanks for @ChenSammi 's review, the issues has been fixed.


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482918376



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       This command follows the shell of volume (ozone sh volume update/create), so use bucketQuota feels unclear, may be volume's quota of bucket number, or misinterpreted as bucket's quota. So, Here, the "quota" is named similar to HDFS, and more detailed description added.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458688805



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,15 @@
       description = "Owner of of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-sq"},
+      description =
+          "Quota Bytes of the newly created volume (eg. 1GB)")

Review comment:
       ```suggestion
             "Quota in bytes of the newly created volume (eg. 1GB)")
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458648117



##########
File path: hadoop-hdds/interface-client/src/main/proto/proto.lock
##########
@@ -1935,4 +1935,4 @@
       }
     }
   ]
-}
+}

Review comment:
       Seems we could not modify here :wink: 




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-663341909


   Thanks for @cxorm 's review. I had revised the review issues. Could you take another look？


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459468457



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Similar to [setOwner](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1697), there already exists [setQuota](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1727). If none of this is needed, we can separate PR to clean up the useless methods.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662458863


   Thanks @cxorm for your review. I have fixed all the problems mentioned above. The command line display has also changed(As shows below).  Could you please take another look？ 
   ![image](https://user-images.githubusercontent.com/13825159/88183192-ba70ba80-cc63-11ea-9ff3-0e1c8b7b0717.png)
   
   


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-662955406


   > Thanks @captainzmc for the work.
   > Overall the functionality-part is good and some suggestions is added inline, test-related-part would be reviewed soon.
   > 
   > Here I have two Questions IMHO.
   > 
   > (1) Could we let the variable-pair be consistent ?
   > I think it would better to use one of (`storagespaceQuota`/ `namespaceQuota`) and (`quotaInBytes`/ `quotaInCounts`).
   > If it have other purpose to use both, please let me know.
   > 
   > (2) Would we add limitation of creating volume and bucket in later patch ?
   > I deployed this patch on my machine, and created volume without setting both of quota.
   > I found that the storage of volume exceeds my local-storage.
   > And I found we could create bucket even if the quota of bucket is -1.
   
   Thanks for @cxorm 's feedback.
   1. I will revise the unified naming and other review issues as soon as possible.
   2. Currently, quota can only be set, but does not take effect. [This task lists plans and designs](https://issues.apache.org/jira/browse/HDDS-541). I will continue to improve this part in the future.
   


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459200596



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -122,8 +136,13 @@ public String getQuota() {
       return this;
     }
 
-    public VolumeArgs.Builder setQuota(String quota) {
-      this.volumeQuota = quota;
+    public VolumeArgs.Builder setStoragespaceQuota(String quota) {
+      this.storagespaceQuota = quota;
+      return this;
+    }
+
+    public VolumeArgs.Builder setNamespaceQuotaQuota(long quota) {

Review comment:
       ```suggestion
       public VolumeArgs.Builder setNamespaceQuota(long quota) {
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459214380



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java
##########
@@ -149,6 +150,13 @@ public static String createStartupShutdownMessage(VersionInfo versionInfo,
         "  java = " + System.getProperty("java.version"));
   }
 
+  /**
+   * The same as String.format(Locale.ENGLISH, format, objects).
+   */
+  public static String format(final String format, final Object... objects) {
+    return String.format(Locale.ENGLISH, format, objects);
+  }
+

Review comment:
       I didn't found usage of this method in all places.
   Could you tell me why we add this part if I miss something ? 




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459245623



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.
+  private long namespaceQuota;
+  // Quota to decide how many storage space will be used in bytes.
+  private long storagespaceQuota;
+  private RawStorageSpaceQuota rawStoragespaceQuota;
+
+  private static class RawStorageSpaceQuota {

Review comment:
       Here we could add comment to the class `RawStorageSpaceQuota` IMHO.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458706466



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,15 @@
       description = "Owner of of the volume")

Review comment:
       ```suggestion
         description = "Owner of the 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459194878



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManager.java
##########
@@ -49,10 +49,12 @@ void setOwner(String volume, String owner)
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
+   * @param storagespaceQuota - Quota in bytes.
    * @throws IOException
    */
-  void setQuota(String volume, long quota) throws IOException;
+  void setQuota(String volume, long namespaceQuota,

Review comment:
       This part is the same as comments on `OzoneManager.java `




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458675987



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -204,10 +215,18 @@ public String getOwner() {
    *
    * @return quotaInBytes
    */
-  public long getQuota() {
-    return quotaInBytes;
+  public long getStoragespaceQuota() {
+    return storagespaceQuota;
   }
 
+  /**
+   * Returns Bucket Quota count allocated for the Volume.

Review comment:
       Suggestion :
   "Returns quota of bucket counts allocated for the 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459202326



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -33,25 +33,30 @@
 
   private final String admin;
   private final String owner;
-  private final String quota;
+  private final String storagespaceQuota;
+  private final long namespaceQuota;
   private final List<OzoneAcl> acls;
   private Map<String, String> metadata;
 
   /**
    * Private constructor, constructed via builder.
    * @param admin Administrator's name.
    * @param owner Volume owner's name
-   * @param quota Volume Quota.
+   * @param storagespaceQuota Volume Quota.
+   * @param namespaceQuota Volume Quota.

Review comment:
       I think we could update the description of the parameter consistent with shell-command IMHO.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459249757



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,25 +133,28 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param storagespaceQuotaStr Storage space Quota String
+   * @param namespaceQuota namespace Quota
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String storagespaceQuotaStr,
+                                      long namespaceQuota) {

Review comment:
       ```suggestion
     public static OzoneQuota parseQuota(String storagespaceQuotaStr,
         long namespaceQuota) {
   ```




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459468457



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Similar to [setOwner](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1697), there already exists [setQuota](https://github.com/apache/hadoop-ozone/blob/7dac140024214c2189b72fad0566a0252d63e93c/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1727). If none of this is needed, we can use separate PR to clean up the useless methods.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459260086



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -204,6 +204,16 @@ public static Versioning getVersioning(boolean versioning) {
    */
   public static final long MAX_QUOTA_IN_BYTES = 1024L * 1024 * TB;
 
+  /**
+   * Quota value.
+   */
+  public static final long QUOTA_COUNT_RESET = -1;

Review comment:
       Could you please update the comment to "Quota of bucket counts" and add the description of meaning of "-1" here ?

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,25 +133,28 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param storagespaceQuotaStr Storage space Quota String
+   * @param namespaceQuota namespace Quota
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String storagespaceQuotaStr,
+                                      long namespaceQuota) {

Review comment:
       ```suggestion
     public static OzoneQuota parseQuota(String storagespaceQuotaStr,
         long namespaceQuota) {
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -53,26 +102,28 @@ public long getSize() {
    * @return Unit in MB, GB or TB
    */
   public Units getUnit() {
-    return unit;
+    return this.rawStoragespaceQuota.getUnit();
   }
 
   /**
    * Constructs a default Quota object.
    */
-  public OzoneQuota() {
-    this.size = 0;
-    this.unit = Units.UNDEFINED;
+  private OzoneQuota() {
+    this.namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
+    this.storagespaceQuota = OzoneConsts.MAX_QUOTA_IN_BYTES;
   }
 
   /**
    * Constructor for Ozone Quota.
    *
-   * @param size Long Size
-   * @param unit MB, GB  or TB
+   * @param namespaceQuota long value
+   * @param rawStoragespaceQuota RawStorageSpaceQuota value
    */
-  public OzoneQuota(long size, Units unit) {
-    this.size = size;
-    this.unit = unit;
+  private OzoneQuota(long namespaceQuota,
+                     RawStorageSpaceQuota rawStoragespaceQuota) {

Review comment:
       ```suggestion
     private OzoneQuota(long namespaceQuota,
         RawStorageSpaceQuota rawStoragespaceQuota) {
   ```




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482918376



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       This command follows the shell of volume (ozone sh volume update/create), so use bucketQuota feels unclear, may be volume's quota of bucket number, or misinterpreted as bucket's quota. So, Here, the "quota" is named similar to HDFS, and more detailed description added.
   More detailed naming rules can be found in the [design documentation](https://issues.apache.org/jira/secure/attachment/13010452/Ozone%20Quota%20Design.pdf).




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc edited a comment on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc edited a comment on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-682285487


   Had fixed issues as HDDS-4129 suggested. Use Long.MAX_VALUE to determine if the parameter values for quota are reasonableCC @ChenSammi.


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-668005922


   hi @xiaoyuyao @arp7 This is the subtask of HDDS-541 Ozone Quota Support. Could you help review 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459196128



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {

Review comment:
       ```suggestion
       if (storagespaceQuota != null) {
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459202494



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -73,11 +78,19 @@ public String getOwner() {
   }
 
   /**
-   * Returns Volume Quota.
-   * @return Quota.
+   * Returns Volume Quota in bytes.
+   * @return storagespaceQuota.
    */
-  public String getQuota() {
-    return quota;
+  public String getStoragespaceQuota() {
+    return storagespaceQuota;
+  }
+
+  /**
+   * Returns Volume Quota in counts.

Review comment:
       ```suggestion
      * Returns Volume Quota in bucket counts.
   ```




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482894593



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -19,32 +19,93 @@
 package org.apache.hadoop.hdds.client;
 
 import org.apache.hadoop.ozone.OzoneConsts;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.hadoop.ozone.OzoneConsts.GB;
+import static org.apache.hadoop.ozone.OzoneConsts.KB;
+import static org.apache.hadoop.ozone.OzoneConsts.MB;
+import static org.apache.hadoop.ozone.OzoneConsts.TB;
 
 
 /**
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {

Review comment:
       Subsequent bucket quota is also intended to use this class, so we might need a more generic name here.




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



---------------------------------------------------------------------
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 pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-685179614


   Thank you @captainzmc for updating the PR. 
   Could you rebase it with latest master branch ?
   
   This PR looks good to me, +1 (cc @ChenSammi )


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] ChenSammi merged pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233


   


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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-671958313


   > Thanks @captainzmc for working on this patch.
   > 
   > This review is about test.
   > Some suggestions are added inline.
   
   Thanks for @cxorm’s  review. Fixed review issues.


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458648117



##########
File path: hadoop-hdds/interface-client/src/main/proto/proto.lock
##########
@@ -1935,4 +1935,4 @@
       }
     }
   ]
-}
+}

Review comment:
       Seems we could not modify here : )




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r469003376



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -101,10 +100,11 @@ void createVolume(String volumeName, VolumeArgs args)
   /**
    * Set Volume Quota.
    * @param volumeName Name of the Volume
-   * @param quota Quota to be set for the Volume
+   * @param quotaInBytes The maximum size this volume can be used.
+   * @param quotaInCounts The maximum number of buckets in this volume.
    * @throws IOException
    */
-  void setVolumeQuota(String volumeName, OzoneQuota quota)
+  void setVolumeQuota(String volumeName, long quotaInBytes, long quotaInCounts)

Review comment:
       Thanks @cxorm  for the discussion, that's a good question.
   The previous feature of Ozone on Set quota is virtual, and this interface won't have any actual effect. This feature is incomplete, so no one will use it before.
   One way to think about it is that this is the new API, and subsequent futures will gradually add new interfaces including clrQuota, etc. Users who want to use full functionality of quota need to update the client.
   So I think we can think of this part as a new interface without regard to backward compatibility.




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



---------------------------------------------------------------------
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 pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-669071164


   > Update PR, like [HDDS-3991](https://issues.apache.org/jira/browse/HDDS-3991), Ignore protobuf lock files.
   > Hi @cxorm. Previous issues has been solved. Could you help take another look？
   
   Sorry for late reply, I would take another look.


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459222501



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/UpdateVolumeHandler.java
##########
@@ -39,22 +40,34 @@
       description = "Owner of the volume to set")
   private String ownerName;
 
-  @Option(names = {"--quota"},
-      description = "Quota of the volume to set"
-          + "(eg. 1G)")
-  private String quota;
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the volume to set (eg. 1GB)")
+  private String storagespaceQuota;
+
+  @Option(names = {"--quota", "-q"},
+      description = "Bucket counts of the volume to set (eg. 5)")
+  private long namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
 
   @Override
   protected void execute(OzoneClient client, OzoneAddress address)
       throws IOException {
-
     String volumeName = address.getVolumeName();
-
     OzoneVolume volume = client.getObjectStore().getVolume(volumeName);
-    if (quota != null && !quota.isEmpty()) {
-      volume.setQuota(OzoneQuota.parseQuota(quota));
+
+    long spaceQuota = volume.getStoragespaceQuota();
+    long countQuota = volume.getNamespaceQuota();
+
+    if (storagespaceQuota != null && !storagespaceQuota.isEmpty()) {
+      spaceQuota = OzoneQuota.parseQuota(storagespaceQuota,
+          namespaceQuota).getStoragespaceQuota();
+    }
+    if (namespaceQuota >= 0) {
+      countQuota = namespaceQuota;
     }
 
+    volume.setQuota(
+        OzoneQuota.getOzoneQuota(spaceQuota, countQuota));

Review comment:
       We could not violate `checkstyle` if we use one line here.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-672572309


   > Thanks @captainzmc for working on this.
   > 
   > This review is mainly about API, and related suggestions are added inline.
   > 
   > Also, there is discussion on naming parameter of CLI in design-doc.
   
   Hi yisheng, I have replied in design-Doc. We've talked about this before. Finally, we refer to the [parameter of HDFS quota](https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HdfsQuotaAdminGuide.html#Administrative_Commands) so as to maximize retention of user habits.
   


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459200918



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {
+      volumeArgsBuilder.setStoragespaceQuota(storagespaceQuota);
     }
+
+    volumeArgsBuilder.setNamespaceQuotaQuota(namespaceQuota);

Review comment:
       The function name seems a little redundant.
   
   Suggestion is commented on `VolumeArgs.java`.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458674407



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -248,12 +267,14 @@ public boolean setOwner(String userName) throws IOException {
 
   /**
    * Sets/Changes the quota of this Volume.
-   * @param quota new quota
-   * @throws IOException
+   *
+   * @param quota@throws IOException

Review comment:
       Suggestion :
   Separate the annotations and add the comments.




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459266753



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -204,6 +204,16 @@ public static Versioning getVersioning(boolean versioning) {
    */
   public static final long MAX_QUOTA_IN_BYTES = 1024L * 1024 * TB;
 
+  /**
+   * Quota value.
+   */
+  public static final long QUOTA_COUNT_RESET = -1;

Review comment:
       Could you please update the comment to "Quota of bucket counts" and add the description of meaning of "-1" here ?

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.
+  private long namespaceQuota;
+  // Quota to decide how many storage space will be used in bytes.
+  private long storagespaceQuota;
+  private RawStorageSpaceQuota rawStoragespaceQuota;
+
+  private static class RawStorageSpaceQuota {

Review comment:
       The RawStorageSpace is a little confuse. Here we could add comment addressed to the class `RawStorageSpaceQuota` IMHO.

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManagerImpl.java
##########
@@ -273,12 +272,14 @@ private void setOwnerCommitToDB(UserVolumeInfo oldOwnerVolumeList,
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in count for bucket.
+   * @param storagespaceQuota - Quota in bytes.
    *
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       This part is the same as comments on `OzoneManager.java`

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -25,26 +25,75 @@
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {
 
   public static final String OZONE_QUOTA_BYTES = "BYTES";
+  public static final String OZONE_QUOTA_KB = "KB";
   public static final String OZONE_QUOTA_MB = "MB";
   public static final String OZONE_QUOTA_GB = "GB";
   public static final String OZONE_QUOTA_TB = "TB";
 
-  private Units unit;
-  private long size;
-
   /** Quota Units.*/
   public enum Units {UNDEFINED, BYTES, KB, MB, GB, TB}
 
+  // Quota to decide how many buckets or keys can be created.

Review comment:
       ```suggestion
     // Quota to decide how many buckets can be created.
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/StringUtils.java
##########
@@ -149,6 +150,13 @@ public static String createStartupShutdownMessage(VersionInfo versionInfo,
         "  java = " + System.getProperty("java.version"));
   }
 
+  /**
+   * The same as String.format(Locale.ENGLISH, format, objects).
+   */
+  public static String format(final String format, final Object... objects) {
+    return String.format(Locale.ENGLISH, format, objects);
+  }
+

Review comment:
       I didn't found usage of this method in all places.
   Could you tell me why we add this part if I miss something ?

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {

Review comment:
       ```suggestion
       if (storagespaceQuota != null) {
   ```

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -33,25 +33,30 @@
 
   private final String admin;
   private final String owner;
-  private final String quota;
+  private final String storagespaceQuota;
+  private final long namespaceQuota;
   private final List<OzoneAcl> acls;
   private Map<String, String> metadata;
 
   /**
    * Private constructor, constructed via builder.
    * @param admin Administrator's name.
    * @param owner Volume owner's name
-   * @param quota Volume Quota.
+   * @param storagespaceQuota Volume Quota.
+   * @param namespaceQuota Volume Quota.

Review comment:
       I think we could update the description of the parameter consistent with shell-command IMHO.

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,25 +133,28 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param storagespaceQuotaStr Storage space Quota String
+   * @param namespaceQuota namespace Quota
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String storagespaceQuotaStr,
+                                      long namespaceQuota) {

Review comment:
       ```suggestion
     public static OzoneQuota parseQuota(String storagespaceQuotaStr,
         long namespaceQuota) {
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -53,26 +102,28 @@ public long getSize() {
    * @return Unit in MB, GB or TB
    */
   public Units getUnit() {
-    return unit;
+    return this.rawStoragespaceQuota.getUnit();
   }
 
   /**
    * Constructs a default Quota object.
    */
-  public OzoneQuota() {
-    this.size = 0;
-    this.unit = Units.UNDEFINED;
+  private OzoneQuota() {
+    this.namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
+    this.storagespaceQuota = OzoneConsts.MAX_QUOTA_IN_BYTES;
   }
 
   /**
    * Constructor for Ozone Quota.
    *
-   * @param size Long Size
-   * @param unit MB, GB  or TB
+   * @param namespaceQuota long value
+   * @param rawStoragespaceQuota RawStorageSpaceQuota value
    */
-  public OzoneQuota(long size, Units unit) {
-    this.size = size;
-    this.unit = unit;
+  private OzoneQuota(long namespaceQuota,
+                     RawStorageSpaceQuota rawStoragespaceQuota) {

Review comment:
       ```suggestion
     private OzoneQuota(long namespaceQuota,
         RawStorageSpaceQuota rawStoragespaceQuota) {
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -143,57 +204,53 @@ public static OzoneQuota parseQuota(String quotaString) {
       throw new IllegalArgumentException("Quota cannot be negative.");
     }
 
-    return new OzoneQuota(nSize, currUnit);
+    return new OzoneQuota(namespaceQuota,
+        new RawStorageSpaceQuota(currUnit, nSize));
   }
 
 
-  /**
-   * Returns size in Bytes or -1 if there is no Quota.
-   */
-  public long sizeInBytes() {
-    switch (this.unit) {
-    case BYTES:
-      return this.getSize();
-    case MB:
-      return this.getSize() * OzoneConsts.MB;
-    case GB:
-      return this.getSize() * OzoneConsts.GB;
-    case TB:
-      return this.getSize() * OzoneConsts.TB;
-    case UNDEFINED:
-    default:
-      return -1;
-    }
-  }
-
   /**
    * Returns OzoneQuota corresponding to size in bytes.
    *
-   * @param sizeInBytes size in bytes to be converted
+   * @param quotaInBytes in bytes to be converted
+   * @param quotaInCounts in counts to be converted
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota getOzoneQuota(long sizeInBytes) {
+  public static OzoneQuota getOzoneQuota(long quotaInBytes,
+                                         long quotaInCounts) {

Review comment:
       ```suggestion
     public static OzoneQuota getOzoneQuota(long quotaInBytes,
         long quotaInCounts) {
   ```

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManager.java
##########
@@ -49,10 +49,12 @@ void setOwner(String volume, String owner)
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
+   * @param storagespaceQuota - Quota in bytes.
    * @throws IOException
    */
-  void setQuota(String volume, long quota) throws IOException;
+  void setQuota(String volume, long namespaceQuota,

Review comment:
       This part is the same as comments on `OzoneManager.java`
   
   

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/UpdateVolumeHandler.java
##########
@@ -39,22 +40,34 @@
       description = "Owner of the volume to set")
   private String ownerName;
 
-  @Option(names = {"--quota"},
-      description = "Quota of the volume to set"
-          + "(eg. 1G)")
-  private String quota;
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the volume to set (eg. 1GB)")
+  private String storagespaceQuota;
+
+  @Option(names = {"--quota", "-q"},
+      description = "Bucket counts of the volume to set (eg. 5)")
+  private long namespaceQuota = OzoneConsts.QUOTA_COUNT_RESET;
 
   @Override
   protected void execute(OzoneClient client, OzoneAddress address)
       throws IOException {
-
     String volumeName = address.getVolumeName();
-
     OzoneVolume volume = client.getObjectStore().getVolume(volumeName);
-    if (quota != null && !quota.isEmpty()) {
-      volume.setQuota(OzoneQuota.parseQuota(quota));
+
+    long spaceQuota = volume.getStoragespaceQuota();
+    long countQuota = volume.getNamespaceQuota();
+
+    if (storagespaceQuota != null && !storagespaceQuota.isEmpty()) {
+      spaceQuota = OzoneQuota.parseQuota(storagespaceQuota,
+          namespaceQuota).getStoragespaceQuota();
+    }
+    if (namespaceQuota >= 0) {
+      countQuota = namespaceQuota;
     }
 
+    volume.setQuota(
+        OzoneQuota.getOzoneQuota(spaceQuota, countQuota));

Review comment:
       We could not violate `checkstyle` if we use one line here.
   

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -1706,21 +1706,24 @@ public boolean setOwner(String volume, String owner) throws IOException {
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota  - Quota in bytes.
+   * @param storagespceQuota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
-    if (isAclEnabled) {
+  public void setQuota(String volume, long namespaceQuota,

Review comment:
       Instead of `OzoneManager.java`, this request is processed in `OMVolumeSetQuotaRequest.java` .
   
   We could not update code-snippet here.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -122,8 +136,13 @@ public String getQuota() {
       return this;
     }
 
-    public VolumeArgs.Builder setQuota(String quota) {
-      this.volumeQuota = quota;
+    public VolumeArgs.Builder setStoragespaceQuota(String quota) {
+      this.storagespaceQuota = quota;
+      return this;
+    }
+
+    public VolumeArgs.Builder setNamespaceQuotaQuota(long quota) {

Review comment:
       ```suggestion
       public VolumeArgs.Builder setNamespaceQuota(long quota) {
   ```

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -58,9 +62,12 @@ protected void execute(OzoneClient client, OzoneAddress address)
     VolumeArgs.Builder volumeArgsBuilder = VolumeArgs.newBuilder()
         .setAdmin(adminName)
         .setOwner(ownerName);
-    if (quota != null) {
-      volumeArgsBuilder.setQuota(quota);
+    if (storagespaceQuota!= null) {
+      volumeArgsBuilder.setStoragespaceQuota(storagespaceQuota);
     }
+
+    volumeArgsBuilder.setNamespaceQuotaQuota(namespaceQuota);

Review comment:
       The function name seems a little redundant.
   
   Suggestion is commented on `VolumeArgs.java`.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -73,11 +78,19 @@ public String getOwner() {
   }
 
   /**
-   * Returns Volume Quota.
-   * @return Quota.
+   * Returns Volume Quota in bytes.
+   * @return storagespaceQuota.
    */
-  public String getQuota() {
-    return quota;
+  public String getStoragespaceQuota() {
+    return storagespaceQuota;
+  }
+
+  /**
+   * Returns Volume Quota in counts.

Review comment:
       ```suggestion
      * Returns Volume Quota in bucket counts.
   ```




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r481477852



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -101,10 +100,11 @@ void createVolume(String volumeName, VolumeArgs args)
   /**
    * Set Volume Quota.
    * @param volumeName Name of the Volume
-   * @param quota Quota to be set for the Volume
+   * @param quotaInBytes The maximum size this volume can be used.
+   * @param quotaInCounts The maximum number of buckets in this volume.
    * @throws IOException
    */
-  void setVolumeQuota(String volumeName, OzoneQuota quota)
+  void setVolumeQuota(String volumeName, long quotaInBytes, long quotaInCounts)

Review comment:
       > The previous feature of Ozone on Set quota is virtual, and this interface won't have any actual effect. This feature is incomplete, so no one will use it before.
   
   I am fine with 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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459194918



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManagerImpl.java
##########
@@ -273,12 +272,14 @@ private void setOwnerCommitToDB(UserVolumeInfo oldOwnerVolumeList,
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in count for bucket.
+   * @param storagespaceQuota - Quota in bytes.
    *
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
+  public void setQuota(String volume, long namespaceQuota,
+                       long storagespaceQuota) throws IOException {

Review comment:
       This part is the same as comments on `OzoneManager.java`
   
   




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458643914



##########
File path: hadoop-hdds/docs/content/shell/VolumeCommands.md
##########
@@ -103,12 +104,13 @@ The volume update command allows changing of owner and quota on a given volume.
 
 | Arguments                      |  Comment                                |
 |--------------------------------|-----------------------------------------|
-| -q, \-\-quota                    | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                    |
-| -u, \-\-user                     |  Optional, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |
+| -sq, \-\-spaceQuota             | Optional, This argument that specifies the maximum size this volume can use in the Ozone cluster.                                        |
+| -q, \-\-quota                  | Optional, This argument that specifies the number of bucket in this volume can use in the Ozone cluster.                                        |
+| -u, \-\-user                   | Optional, The name of the user who owns this volume. This user can create, buckets and keys on this volume.                                       |

Review comment:
       ```suggestion
   | -u, \-\-user                   | Optional, The name of the user who owns this volume. This user can create buckets and keys on this 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] ChenSammi commented on a change in pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r482675817



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -53,26 +114,27 @@ public long getSize() {
    * @return Unit in MB, GB or TB
    */
   public Units getUnit() {
-    return unit;
+    return this.rawQuotaInBytes.getUnit();
   }
 
   /**
    * Constructs a default Quota object.
    */
-  public OzoneQuota() {
-    this.size = 0;
-    this.unit = Units.UNDEFINED;
+  private OzoneQuota() {
+    this.quotaInCounts = OzoneConsts.QUOTA_RESET;
+    this.quotaInBytes = OzoneConsts.QUOTA_RESET;

Review comment:
       rawQuotaInBytes initialization is missed.

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,118 +144,130 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();

Review comment:
       need space before "+"

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -19,32 +19,93 @@
 package org.apache.hadoop.hdds.client;
 
 import org.apache.hadoop.ozone.OzoneConsts;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.hadoop.ozone.OzoneConsts.GB;
+import static org.apache.hadoop.ozone.OzoneConsts.KB;
+import static org.apache.hadoop.ozone.OzoneConsts.MB;
+import static org.apache.hadoop.ozone.OzoneConsts.TB;
 
 
 /**
  * represents an OzoneQuota Object that can be applied to
  * a storage volume.
  */
-public class OzoneQuota {
+public final class OzoneQuota {

Review comment:
       Shall we rename this class name to VolumeQuota?

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,118 +144,130 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param quotaInBytes Volume quota in bytes
+   * @param quotaInCounts Volume quota in counts
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String quotaInBytes,
+      long quotaInCounts) {
 
-    if ((quotaString == null) || (quotaString.isEmpty())) {
+    if ((quotaInBytes == null) || (quotaInBytes.isEmpty())) {
       throw new IllegalArgumentException(
           "Quota string cannot be null or empty.");
     }
 
-    String uppercase = quotaString.toUpperCase().replaceAll("\\s+", "");
+    String uppercase = quotaInBytes.toUpperCase()
+        .replaceAll("\\s+", "");
     String size = "";
-    int nSize;
+    long nSize = 0;
     Units currUnit = Units.MB;
-    boolean found = false;
-    if (uppercase.endsWith(OZONE_QUOTA_MB)) {
-      size = uppercase
-          .substring(0, uppercase.length() - OZONE_QUOTA_MB.length());
-      currUnit = Units.MB;
-      found = true;
-    }
+    long quotaMultiplyExact = 0;
 
-    if (uppercase.endsWith(OZONE_QUOTA_GB)) {
-      size = uppercase
-          .substring(0, uppercase.length() - OZONE_QUOTA_GB.length());
-      currUnit = Units.GB;
-      found = true;
-    }
+    try {
+      if (uppercase.endsWith(OZONE_QUOTA_KB)) {
+        size = uppercase
+            .substring(0, uppercase.length() - OZONE_QUOTA_KB.length());
+        currUnit = Units.KB;
+        quotaMultiplyExact = Math.multiplyExact(Long.parseLong(size), KB);
+      }
 
-    if (uppercase.endsWith(OZONE_QUOTA_TB)) {
-      size = uppercase
-          .substring(0, uppercase.length() - OZONE_QUOTA_TB.length());
-      currUnit = Units.TB;
-      found = true;
-    }
+      if (uppercase.endsWith(OZONE_QUOTA_MB)) {
+        size = uppercase
+            .substring(0, uppercase.length() - OZONE_QUOTA_MB.length());
+        currUnit = Units.MB;
+        quotaMultiplyExact = Math.multiplyExact(Long.parseLong(size), MB);
+      }
 
-    if (uppercase.endsWith(OZONE_QUOTA_BYTES)) {
-      size = uppercase
-          .substring(0, uppercase.length() - OZONE_QUOTA_BYTES.length());
-      currUnit = Units.BYTES;
-      found = true;
-    }
+      if (uppercase.endsWith(OZONE_QUOTA_GB)) {
+        size = uppercase
+            .substring(0, uppercase.length() - OZONE_QUOTA_GB.length());
+        currUnit = Units.GB;
+        quotaMultiplyExact = Math.multiplyExact(Long.parseLong(size), GB);
+      }
 
-    if (!found) {
-      throw new IllegalArgumentException("Quota unit not recognized. " +
-          "Supported values are BYTES, MB, GB and TB.");
+      if (uppercase.endsWith(OZONE_QUOTA_TB)) {
+        size = uppercase
+            .substring(0, uppercase.length() - OZONE_QUOTA_TB.length());
+        currUnit = Units.TB;
+        quotaMultiplyExact = Math.multiplyExact(Long.parseLong(size), TB);
+      }
+
+      if (uppercase.endsWith(OZONE_QUOTA_BYTES)) {
+        size = uppercase
+            .substring(0, uppercase.length() - OZONE_QUOTA_BYTES.length());
+        currUnit = Units.BYTES;
+        quotaMultiplyExact = Math.multiplyExact(Long.parseLong(size), 1L);
+      }
+      nSize = Long.parseLong(size);
+    } catch (NumberFormatException e) {
+      throw new IllegalArgumentException("Invalid values for quota, to ensure" +
+          " that the Quota format is legal(supported values are BYTES, KB, " +
+          "MB, GB and TB).");
+    } catch  (ArithmeticException e) {
+      LOG.debug("long overflow:\n{}", quotaMultiplyExact);
+      throw new IllegalArgumentException("Invalid values for quota, the quota" +
+          " value cannot be greater than Long.MAX_VALUE BYTES");
     }
 
-    nSize = Integer.parseInt(size);
     if (nSize < 0) {
       throw new IllegalArgumentException("Quota cannot be negative.");
     }
 
-    return new OzoneQuota(nSize, currUnit);
+    return new OzoneQuota(quotaInCounts,
+        new RawQuotaInBytes(currUnit, nSize));
   }
 
 
-  /**
-   * Returns size in Bytes or -1 if there is no Quota.
-   */
-  public long sizeInBytes() {
-    switch (this.unit) {
-    case BYTES:
-      return this.getSize();
-    case MB:
-      return this.getSize() * OzoneConsts.MB;
-    case GB:
-      return this.getSize() * OzoneConsts.GB;
-    case TB:
-      return this.getSize() * OzoneConsts.TB;
-    case UNDEFINED:
-    default:
-      return -1;
-    }
-  }
-
   /**
    * Returns OzoneQuota corresponding to size in bytes.
    *
-   * @param sizeInBytes size in bytes to be converted
+   * @param quotaInBytes in bytes to be converted
+   * @param quotaInCounts in counts to be converted
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota getOzoneQuota(long sizeInBytes) {
+  public static OzoneQuota getOzoneQuota(long quotaInBytes,
+      long quotaInCounts) {
     long size;
     Units unit;
-    if (sizeInBytes % OzoneConsts.TB == 0) {
-      size = sizeInBytes / OzoneConsts.TB;
+    if (quotaInBytes % TB == 0) {
+      size = quotaInBytes / TB;
       unit = Units.TB;
-    } else if (sizeInBytes % OzoneConsts.GB == 0) {
-      size = sizeInBytes / OzoneConsts.GB;
+    } else if (quotaInBytes % GB == 0) {
+      size = quotaInBytes / GB;
       unit = Units.GB;
-    } else if (sizeInBytes % OzoneConsts.MB == 0) {
-      size = sizeInBytes / OzoneConsts.MB;
+    } else if (quotaInBytes % MB == 0) {
+      size = quotaInBytes / MB;
       unit = Units.MB;
+    } else if (quotaInBytes % KB == 0) {
+      size = quotaInBytes / KB;
+      unit = Units.KB;
     } else {
-      size = sizeInBytes;
+      size = quotaInBytes;
       unit = Units.BYTES;
     }
-    return new OzoneQuota((int)size, unit);
+    return new OzoneQuota(quotaInCounts, new RawQuotaInBytes(unit, size));
+  }
+
+  public long getQuotaInCounts() {
+    return quotaInCounts;
+  }
+
+  public long getQuotaInBytes() {
+    return quotaInBytes;
   }
 
   @Override
   public String toString() {
-    return size + " " + unit;
+    return "Bytes Quota: " + rawQuotaInBytes.toString() + "\n" +

Review comment:
       Bytes Quota -> Space Bytes Quota
   Counts Quota -> Bucket Counts Quota

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java
##########
@@ -82,118 +144,130 @@ public OzoneQuota(long size, Units unit) {
    * @return string representation of quota
    */
   public static String formatQuota(OzoneQuota quota) {
-    return String.valueOf(quota.size) + quota.unit;
+    return String.valueOf(quota.getRawSize())+ quota.getUnit();
   }
 
   /**
    * Parses a user provided string and returns the
    * Quota Object.
    *
-   * @param quotaString Quota String
+   * @param quotaInBytes Volume quota in bytes
+   * @param quotaInCounts Volume quota in counts
    *
    * @return OzoneQuota object
    */
-  public static OzoneQuota parseQuota(String quotaString) {
+  public static OzoneQuota parseQuota(String quotaInBytes,
+      long quotaInCounts) {
 
-    if ((quotaString == null) || (quotaString.isEmpty())) {
+    if ((quotaInBytes == null) || (quotaInBytes.isEmpty())) {

Review comment:
       Use Strings.isNullOrEmpty instead.

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},
+      description = "Quota in bytes of the newly created volume (eg. 1GB)")
+  private String quotaInBytes;
+
   @Option(names = {"--quota", "-q"},

Review comment:
       --quota  -> bucketQuota
   -q -> -bq

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/UpdateVolumeHandler.java
##########
@@ -39,22 +40,33 @@
       description = "Owner of the volume to set")
   private String ownerName;
 
-  @Option(names = {"--quota"},
-      description = "Quota of the volume to set"
-          + "(eg. 1G)")
-  private String quota;
+  @Option(names = {"--spaceQuota", "-s"},

Review comment:
       same as above

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -204,6 +204,16 @@ public static Versioning getVersioning(boolean versioning) {
    */
   public static final long MAX_QUOTA_IN_BYTES = 1024L * 1024 * TB;
 
+  /**
+   * Quota RESET default is -1, which means it does not take effect.

Review comment:
       which means it does not take effect -> which means quota is not set

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,13 @@
       description = "Owner of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-s"},

Review comment:
       -s -> -sq




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458690505



##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/volume/CreateVolumeHandler.java
##########
@@ -40,10 +41,15 @@
       description = "Owner of of the volume")
   private String ownerName;
 
+  @Option(names = {"--spaceQuota", "-sq"},
+      description =
+          "Quota Bytes of the newly created volume (eg. 1GB)")
+  private String storagespaceQuota;
+
   @Option(names = {"--quota", "-q"},
       description =
-          "Quota of the newly created volume (eg. 1G)")
-  private String quota;
+          "Quota count of the newly created volume (eg. 5)")

Review comment:
       Correct me if I miss something.
   Would it be better to update the description to "Bucket counts of the newly created volume (eg. 5) " ?




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459194878



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManager.java
##########
@@ -49,10 +49,12 @@ void setOwner(String volume, String owner)
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in counts.
+   * @param storagespaceQuota - Quota in bytes.
    * @throws IOException
    */
-  void setQuota(String volume, long quota) throws IOException;
+  void setQuota(String volume, long namespaceQuota,

Review comment:
       This part is the same as comments on `OzoneManager.java `

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManagerImpl.java
##########
@@ -273,12 +272,14 @@ private void setOwnerCommitToDB(UserVolumeInfo oldOwnerVolumeList,
    * Changes the Quota on a volume.
    *
    * @param volume - Name of the volume.
-   * @param quota - Quota in bytes.
+   * @param namespaceQuota - Quota in count for bucket.
+   * @param storagespaceQuota - Quota in bytes.
    *
    * @throws IOException
    */
   @Override
-  public void setQuota(String volume, long quota) throws IOException {
+  public void setQuota(String volume, long namespaceQuota,
+                       long storagespaceQuota) throws IOException {

Review comment:
       This part is the same as comments on OzoneManager.java
   
   




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r459202494



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/VolumeArgs.java
##########
@@ -73,11 +78,19 @@ public String getOwner() {
   }
 
   /**
-   * Returns Volume Quota.
-   * @return Quota.
+   * Returns Volume Quota in bytes.
+   * @return storagespaceQuota.
    */
-  public String getQuota() {
-    return quota;
+  public String getStoragespaceQuota() {
+    return storagespaceQuota;
+  }
+
+  /**
+   * Returns Volume Quota in counts.

Review comment:
       ```suggestion
      * Returns Volume Quota in bucket counts.
   ```




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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org

[GitHub] [hadoop-ozone] captainzmc commented on pull request #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
captainzmc commented on pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#issuecomment-682285487


   Had fixed issues as HDDS-4129 suggested. CC @ChenSammi.


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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r468426519



##########
File path: hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestJsonUtils.java
##########
@@ -32,11 +32,11 @@
 
   @Test
   public void printObjectAsJson() throws IOException {
-    OzoneQuota quota = new OzoneQuota(123, OzoneQuota.Units.MB);
+    OzoneQuota quota = OzoneQuota.parseQuota("123MB", 1000L);
 
     String result = JsonUtils.toJsonStringWithDefaultPrettyPrinter(quota);
 

Review comment:
       I think we should verify `quotaInCounts` here, too.

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
##########
@@ -271,10 +271,10 @@ public void testSetVolumeQuota()
       throws IOException {
     String volumeName = UUID.randomUUID().toString();
     store.createVolume(volumeName);
-    store.getVolume(volumeName).setQuota(
-        OzoneQuota.parseQuota("100000000 BYTES"));
+    store.getVolume(volumeName).setQuota(OzoneQuota.parseQuota("1GB", 0L));
     OzoneVolume volume = store.getVolume(volumeName);
-    Assert.assertEquals(100000000L, volume.getQuota());
+    Assert.assertEquals(1024 * 1024 * 1024,
+        volume.getQuotaInBytes());

Review comment:
       The same as above.
   We should verify two quota attributes if the `OzoneQuota` has two attributes.

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -734,7 +734,8 @@ public void testTempMount() throws Exception {
     // Sanity check
     Assert.assertNull(volumeArgs.getOwner());
     Assert.assertNull(volumeArgs.getAdmin());
-    Assert.assertNull(volumeArgs.getQuota());
+    Assert.assertNull(volumeArgs.getQuotaInBytes());
+    Assert.assertEquals(0, volumeArgs.getQuotaInCounts());

Review comment:
       I think we should verify two quota attributes if the `OzoneQuota` has two attributes.

##########
File path: hadoop-ozone/dist/src/main/smoketest/topology/loaddata.robot
##########
@@ -25,7 +25,7 @@ Test Timeout        5 minutes
 
 *** Test Cases ***
 Create a volume, bucket and key
-    ${output} =         Execute          ozone sh volume create topvol1 --quota 100TB
+    ${output} =         Execute          ozone sh volume create topvol1

Review comment:
       Why could we not set `spaceQuota` here ?
   
   




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



---------------------------------------------------------------------
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 #1233: HDDS-3725. Ozone sh volume client support quota option.

Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1233:
URL: https://github.com/apache/hadoop-ozone/pull/1233#discussion_r458649610



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -61,9 +61,13 @@
    */
   private String owner;
   /**
-   * Quota allocated for the Volume.
+   * Bytes Quota allocated for the Volume.

Review comment:
       Suggestion :
   "Quota of bytes allocated for the 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



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org