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/09/09 01:36:16 UTC
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #1296: HDDS-4053. Volume space: add quotaUsageInBytes and update it when write and delete key.
cxorm commented on a change in pull request #1296:
URL: https://github.com/apache/hadoop-ozone/pull/1296#discussion_r484607389
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -269,6 +269,7 @@ private OzoneConsts() {
public static final String KEY = "key";
public static final String SRC_KEY = "srcKey";
public static final String DST_KEY = "dstKey";
+ public static final String QUOTA_USAGE_IN_BYTES = "quotaUsageInBytes";
Review comment:
How do we use `QUOTA_USAGE = "quotaUsage"` here ?
Could you please update the rest part if you agree the idea ?
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
##########
@@ -46,6 +47,7 @@
private long quotaInBytes;
private long quotaInCounts;
private final OmOzoneAclMap aclMap;
+ private LongAdder quotaUsageInBytes = new LongAdder();
Review comment:
I'm fine with the `LongAdder` here.
Out of curiosity,
Could you be so kind to tell me why we use it here ?
(Or please describe the bad pattern if we use just `long` or `Long`)
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -360,6 +360,8 @@ message VolumeInfo {
optional uint64 updateID = 9;
optional uint64 modificationTime = 10;
optional uint64 quotaInCounts = 11;
+ optional uint64 quotaUsageInBytes = 12;
+
Review comment:
```suggestion
optional uint64 quotaUsage = 12;
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
##########
@@ -222,6 +218,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
acquireLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
volumeName, bucketName);
+
Review comment:
```suggestion
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##########
@@ -143,14 +143,25 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
keyName)),
new CacheValue<>(Optional.absent(), trxnLogIndex));
+ long totalKeySize = 0;
Review comment:
```suggestion
long quotaReleased = 0;
```
We could name the variable in a proper meaning IMHO,
feel free to rename it if you have an idea.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -151,21 +154,32 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ long totalKeySize = 0;
Review comment:
```suggestion
long quotaReleased = 0;
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java
##########
@@ -292,14 +289,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
bucketName, Optional.absent(), Optional.of(missingParentInfos),
trxnLogIndex);
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ long updateNum = newLocationList.size() * scmBlockSize
Review comment:
```suggestion
long preAllocatedSpace = newLocationList.size() * scmBlockSize
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##########
@@ -143,14 +143,25 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
keyName)),
new CacheValue<>(Optional.absent(), trxnLogIndex));
+ long totalKeySize = 0;
+ int keyFactor = omKeyInfo.getFactor().getNumber();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ OmKeyLocationInfoGroup keyLocationGroup =
+ omKeyInfo.getLatestVersionLocations();
+ for(OmKeyLocationInfo locationInfo: keyLocationGroup.getLocationList()){
+ totalKeySize += locationInfo.getLength() * keyFactor;
+ }
+ // atom update quotaUsage.
Review comment:
```suggestion
// update quotaUsage atomically.
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -151,21 +154,32 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ long totalKeySize = 0;
+ OmVolumeArgs omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+
// Mark all keys which can be deleted, in cache as deleted.
for (OmKeyInfo omKeyInfo : omKeyInfoList) {
omMetadataManager.getKeyTable().addCacheEntry(
new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
omKeyInfo.getKeyName())),
new CacheValue<>(Optional.absent(), trxnLogIndex));
+
+ int keyFactor = omKeyInfo.getFactor().getNumber();
+ OmKeyLocationInfoGroup keyLocationGroup =
+ omKeyInfo.getLatestVersionLocations();
+ for(OmKeyLocationInfo locationInfo: keyLocationGroup.getLocationList()){
+ totalKeySize += locationInfo.getLength() * keyFactor;
+ }
}
+ // atom update quotaUsage.
Review comment:
```suggestion
// update quotaUsage atomically.
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
##########
@@ -295,14 +293,28 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(dbOpenKeyName),
new CacheValue<>(Optional.of(omKeyInfo), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+
+ // Here we refer to the implementation of HDFS:
+ // If the key size is 600MB, when createKey, keyLocationList is 3, and
+ // the every pre-allocated block length is 256MB. If the number of factor
+ // is 3, the total pre-allocated block size is 256MB*3*3.
+ // We will allocate more 256MB*3* 3-600mb *3 = 504MB in advance, and we
+ // will subtract this part when we finally commitKey.
Review comment:
```suggestion
// Here we refer to the implementation of HDFS:
// If the key size is 600MB, when createKey, keyLocationInfo in keyLocationList is 3, and
// the every pre-allocated block length is 256MB. If the number of factor
// is 3, the total pre-allocated block size is 256MB * 3 * 3.
// We will allocate more 256MB * 3 * 3 - 600MB * 3 = 504MB in advance, and we
// will subtract this part when we finally commitKey.
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
##########
@@ -295,14 +293,28 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(dbOpenKeyName),
new CacheValue<>(Optional.of(omKeyInfo), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+
+ // Here we refer to the implementation of HDFS:
+ // If the key size is 600MB, when createKey, keyLocationList is 3, and
+ // the every pre-allocated block length is 256MB. If the number of factor
+ // is 3, the total pre-allocated block size is 256MB*3*3.
+ // We will allocate more 256MB*3* 3-600mb *3 = 504MB in advance, and we
+ // will subtract this part when we finally commitKey.
+ long updateNum = newLocationList.size() * scmBlockSize
Review comment:
```suggestion
long preAllocatedSpace = newLocationList.size() * scmBlockSize
```
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -131,6 +133,18 @@ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
this.modificationTime = Instant.ofEpochMilli(modificationTime);
}
+ @SuppressWarnings("parameternumber")
+ public OzoneVolume(ConfigurationSource conf, ClientProtocol proxy,
+ String name, String admin, String owner, long quotaInBytes,
+ long quotaInCounts, long creationTime, long modificationTime,
+ List<OzoneAcl> acls, Map<String, String> metadata,
+ long quotaUsageInBytes) {
+ this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
+ creationTime, acls, metadata);
+ this.modificationTime = Instant.ofEpochMilli(modificationTime);
+ this.quotaUsageInBytes = quotaUsageInBytes;
Review comment:
```suggestion
this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
creationTime, modificationTime, acls, metadata);
this.quotaUsageInBytes = quotaUsageInBytes;
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -182,8 +179,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(dbOzoneKey),
new CacheValue<>(Optional.of(omKeyInfo), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ int factor = omKeyInfo.getFactor().getNumber();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ long updateNum = omKeyInfo.getDataSize() * factor -
+ locationInfoList.size() * scmBlockSize * factor;
Review comment:
I'm confused about the meaning of the variable.
(Please let me know the meaning of the variable if I miss something.)
Could you please add some comments about it ?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -210,10 +206,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKeyName),
new CacheValue<>(Optional.of(openKeyInfo), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ long updateNum = newLocationList.size() * scmBlockSize
Review comment:
```suggestion
long preAllocatedSpace = newLocationList.size() * scmBlockSize
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java
##########
@@ -207,13 +209,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKey),
new CacheValue<>(Optional.absent(), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ int factor = omKeyInfo.getFactor().getNumber();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ long updateNum = omKeyInfo.getDataSize() * factor -
+ keyArgs.getKeyLocationsList().size() * scmBlockSize * factor;
Review comment:
The same as `OMKeyCommitRequest`
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -210,10 +206,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKeyName),
new CacheValue<>(Optional.of(openKeyInfo), trxnLogIndex));
+ long scmBlockSize = ozoneManager.getScmBlockSize();
+ omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
+ long updateNum = newLocationList.size() * scmBlockSize
+ * openKeyInfo.getFactor().getNumber();
+ // atom update quotaUsage.
Review comment:
```suggestion
// update quotaUsage atomically.
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java
##########
@@ -105,5 +108,10 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
omMetadataManager.getDeletedTable().putWithBatch(batchOperation,
deleteKey, repeatedOmKeyInfo);
}
+
+ // update volume quotaUsageInBytes.
+ omMetadataManager.getVolumeTable().putWithBatch(batchOperation,
+ omMetadataManager.getVolumeKey(omVolumeArgs.getVolume()),
+ omVolumeArgs);
}
}
Review comment:
```suggestion
}
```
----------------------------------------------------------------
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