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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
