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 2022/04/07 15:12:16 UTC
[GitHub] [ozone] captainzmc opened a new pull request, #3286: HDDS-6556. Update usedBytes only when commit key.
captainzmc opened a new pull request, #3286:
URL: https://github.com/apache/ozone/pull/3286
## What changes were proposed in this pull request?
As we disscussed in https://issues.apache.org/jira/browse/HDDS-5867.
The quota we are currently implementing have a bug. When allocate block, we add usedBytes. If the key fails to write, the Clean Open Key Service subtracts the usedBytes. But the bucket may no longer exist when the open key is being deleted, or may have been deleted and re-created with the same name. This causes usedBytes update error.
We can only update the usedByte when commit key. In this way, failed keys are not counted in the usedByte（we‘ll add service in the background to delete them). This is reasonable for the user. This is garbage data and belongs to the Ozone system. And the Ozone system itself cleans this data.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6556
## How was this patch tested?
existing UT, also add new 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1091905512
With the approach suggested here, if we no longer check quota when allocating blocks, I could write a very large key only to find it to beyond the quota when I commit it - this is potentially gigabytes of data. If I am writing something like streaming data, it may be hard to "rewind" the stream to write it all again. It is more user friendly IMO, that when you get a block allocated, its guaranteed to not break the quota as your space is pre-allocated.
If the issue is mainly around dropping a bucket, can we do anything at bucket drop time? Ideally we should not be able to drop a bucket that still contains keys, so could the drop bucket operation check the open key table and manage its open keys and quota accordingly before the drop bucket completes?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1120115439
Thanks @errose28 and @kaijchen for the review and thanks @sodonnel for the advice. There are some PR, which depend on this patch. So let's merge this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1101924134
Thanks @errose28 for your feedback. So let continue open key cleanup implementation. If there are other problems, we can discuss them in the new PR. cc @kaijchen.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If possible, I will change it in this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866283268
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
@captainzmc I don't totally follow the logic in this part, and the corresponding part in OMKeyCommitRequestWithFSO. Could you clarify how this is handling quota calculations when we are/are not overwriting a key, and how versioning be enabled or disabled affects this?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequestWithFSO.java:
##########
@@ -144,7 +144,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long preAllocatedSpace = newLocationList.size()
* ozoneManager.getScmBlockSize()
* openKeyInfo.getReplicationConfig().getRequiredNodes();
- checkBucketQuotaInBytes(omBucketInfo, preAllocatedSpace);
+ long hadAllocatedSpace =
+ openKeyInfo.getLatestVersionLocations().getLocationList().size()
+ * ozoneManager.getScmBlockSize()
+ * openKeyInfo.getReplicationConfig().getRequiredNodes();
Review Comment:
I don't think this will give the correct result for EC. Looks like `QuotaUtil#getReplicatedSize` has logic that might help 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5461. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If possible, I will change 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1100033309
Thank @errose28 and @sodonnel for your feedback. I get your point.
> It still does not prevent someone (or a bad application) writing keys up to the quota, and then abandoning them, filling up the system.
>
> Perhaps we need some sort of "open key + abandoned key" quota to go along with the committed quota. But that starts to make it harder to understand how the whole system works, which is bad for users too!
Agree. At allocate time check quota, failed keys are treated as part of the system. This will not guarantee a user could fill up the system by writing some failed key.
Ozone's current implementation has none of these risks. However, it does not solve the problems encountered by the Clean Open Key Service.
I have an idea here. Currently, all keys need to be delete before Ozone deletes a bucket. In fact, this part of the key need contain the Open key? If yes, can we implement an interface that allows users to clean the Open key under a bucket? Just my idea, and if there's some other way, I'm glad to discuss. cc @errose28 @sodonnel
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1099593096
I like the idea from @errose28, where we check the quota as we allocate each block to an open key. This means each open key cannot exceed the total available quota, but also if someone is writing small keys, they can now write many concurrently with a small quota - before that would have failed as we pre-allocate 256mb * 3 for each open key, even if you only write 1MB.
It still does not prevent someone (or a bad application) writing keys up to the quota, and then abandoning them, filling up the system.
Also, what would happen if an open key fails to allocate a block due to exceeding the quota? Will it be left as an open key and have to wait for a cleanup job?
I am still not sure on this though. The purpose of a quota is to prevent a user from consuming more than a certain amount of disk space on the system. In that respect, surely anything on disk owned by that user, either open, abandoned or committed should count toward the quota?
Perhaps we need some sort of "open key + abandoned key" quota to go along with the committed quota. But that starts to make it harder to understand how the whole system works, which is bad for users too!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1103417426
> @captainzmc are we still planning on continuing this PR? If the approach presented above sounds good, I think it would be better to implement it on its own here instead of mixing it with another PR.
>
> > Each allocate block request checks that existing open key size + the block to be allocated + current quota usage <= max quota. If this condition fails, allocate block fails.
> > Each key commit checks that total key size + current quota usage <= max quota. If so, it updates the quota usage and commits the key.
Sure. today I will update this PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If possible, I will change this in this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5461. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
I had submit a new PR, and had request @kuenishi and @errose28 to review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1091985151
Thanks @sodonnel review and discuss this. My original idea is same with you, but there are some problems.
Ozone is only visible after the key has been written. If the key fails to write, eventually the key is unavailable. In addition, it is rare for a user to write more data than quota (while quota error happened, user needs to increase quota), also writing large keys is very unlikely to happen in practice.
The main reasons for adopting the current way is:
1. From the user's point of view, it is not reasonable to count a failed write key as quota. Because they can't do anything with the failed key. They don't know these failure keys exist. They cannot view, modify, or continue writing. These failed keys are like system garbage in Ozone.
2. Each time we allocate, because we do not know the final size of the key, we increase usedBytes by 256*3MB each time. If the user's quota is only 600MB and the user only wants to write a 1MB key, the write will still fail. This doesn't make sense from the user's point of view because he has enough quota left to write 1MB. (In our production environment we have seen users raise this issue)
3. At present, the only service we have implemented to clean up these failed keys is also background automatic. It's not up to the user to call it. So cleaning these keys is also the work of the Ozone system itself. If we add usedBytes in the allocate block, then there is no way for the user to manually release these usedBytes after the failure, and the user will be confused.
4. Currently, when deleting a bucket, we require the user to clear all keys, but not open keys. At present, it seems that the background asynchronous clean Open key is the most efficient, exposing the interface for users to clean will make the use of Ozone more complicated.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1092407207
Another approach to solve the bucket re-creation problem is to record the bucket ID in open key table.
We should only update quota if the bucket ID is same as recorded.
Currently, for FSO keys, we can traverse `parentID` to find the bucket ID.
For OBJECT/LEGACY keys, `parentID` is not used now, which can be used to store the bucket ID later.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1101630985
Also, once open key cleanup implementation is finished, these open keys will be cleared out fairly frequently (after 24 hours by default), so they will not be taking up extra space in HDDS for too long. Therefore I think we are ok pushing out a solution for this problem for now.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858708728
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java:
##########
@@ -52,7 +52,7 @@ public class OMKeyCreateResponse extends OmKeyResponse {
public OMKeyCreateResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo omKeyInfo, List<OmKeyInfo> parentKeyInfos,
- long openKeySessionID, @Nonnull OmBucketInfo omBucketInfo) {
+ long openKeySessionID, OmBucketInfo omBucketInfo) {
Review Comment:
It was deleted by mistake. I'll change 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1111128571
Hi @captainzmc, one more question here. Beside this, LGTM.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1097573039
> As @sodonnel said, to prevent users writing large keys to DDOS the cluster, we still need to calculate usedBytes while allocate block
I think my explanation for this part may have been confusing. I meant that we would still have a check in allocate block against the quota, and fail the allocation if it caused the current open key size to be larger than the quota. This prevents writing large keys exceeding the quota. This is still a quota check, but it is only a check against already committed data, not open uncommitted data.
> We can also check if there is an Open keys, and if there is, the bucket is not allowed to be deleted.
I'm not sure this is a good idea. Open keys are internal to the OM, and should not block other user operations, whether that is a bucket delete or a block allocate. Only committed data that is visible to the user (like committed keys) should prevent user operations.
> About the problem of writing small files requires at least 3* blocks. I see HDFS also requires at least 3* blocks. So we can be consistent with HDFS and ignore this problem.
+1 for this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1097611819
> We can also check if there is an Open keys, and if there is, the bucket is not allowed to be deleted.
We can delete open keys when the bucket gets deleted.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5461. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
I had submit a new PR #3388, and had request @kuenishi and @errose28 to review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1092051034
> I agree there are problems with the current approach, especially needing to allocate in full block increments. However quota should stop someone from using more than their allocated space on the cluster. What if I start writing massive keys to effectively DDOS the cluster. I can just write and write beyond my quota TB's of data and never commit my keys. I can start many clients to do this in parallel and fill the entire cluster with garbage data.
>
> In some respects, a failed to write key is using space and it could be argued it should count against the quota. It could be argued either way. However the ability for a user to write huge amounts of data beyond their quota, without the system blocking them in some way is bad I think.
Hi @errose28, Updating quota only at commit key time had unexpected problems. So, about problem that caused by [HDDS-5867](https://issues.apache.org/jira/browse/HDDS-5867), is there anything else we need to discuss 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1098247750
> We can delete open keys when the bucket gets deleted.
This would give an incorrect error to a client who is in the process of writing a key while a bucket is deleted. They would get something like "key not found" when doing allocate block or key commit after the bucket is deleted, when they should get "bucket not found".
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r859919660
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
If `keyToDelete` isn't `null`, we shouldn't increase usedNamespace.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc merged pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc merged PR #3286:
URL: https://github.com/apache/ozone/pull/3286
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858329070
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java:
##########
@@ -52,7 +52,7 @@ public class OMKeyCreateResponse extends OmKeyResponse {
public OMKeyCreateResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo omKeyInfo, List<OmKeyInfo> parentKeyInfos,
- long openKeySessionID, @Nonnull OmBucketInfo omBucketInfo) {
+ long openKeySessionID, OmBucketInfo omBucketInfo) {
Review Comment:
Why removing `@Nonnull` 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858331660
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java:
##########
@@ -226,15 +226,9 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKey),
new CacheValue<>(Optional.absent(), trxnLogIndex));
- long scmBlockSize = ozoneManager.getScmBlockSize();
- int factor = omKeyInfo.getReplicationConfig().getRequiredNodes();
omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
- // Block was pre-requested and UsedBytes updated when createKey and
- // AllocatedBlock. The space occupied by the Key shall be based on
- // the actual Key size, and the total Block size applied before should
- // be subtracted.
- long correctedSpace = omKeyInfo.getReplicatedSize() -
- keyArgs.getKeyLocationsList().size() * scmBlockSize * factor;
+
+ long correctedSpace = omKeyInfo.getReplicatedSize();
omBucketInfo.incrUsedBytes(correctedSpace);
Review Comment:
Should check quota and also increase usedNamespace when commit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1098712106
> This would give an incorrect error to a client who is in the process of writing a key while a bucket is deleted. They would get something like "key not found" when doing allocate block or key commit after the bucket is deleted, when they should get "bucket not found".
Yeah that might be a problem.
Overall I think @errose28's idea is better.
Although it is still possible to DDOS the cluster by creating a lot of open keys.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1092038544
> > also writing large keys is very unlikely to happen in practice.
>
> I don't agree with that at all. Large keys, small keys - they will all get written.
I mean here is to write a big key when the quota threshold is near(a normal user can add a quota whenever a quota exception is raised). Unless, of course, the user intends to.
> I agree there are problems with the current approach, especially needing to allocate in full block increments. However quota should stop someone from using more than their allocated space on the cluster. What if I start writing massive keys to effectively DDOS the cluster. I can just write and write beyond my quota TB's of data and never commit my keys. I can start many clients to do this in parallel and fill the entire cluster with garbage data.
> In some respects, a failed to write key is using space and it could be argued it should count against the quota. It could be argued either way. However the ability for a user to write huge amounts of data beyond their quota, without the system blocking them in some way is bad I think.
Agree, it's true that the new implementation does not prevent users with access from writing large keys over and over again.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1096444869
> * Open keys, which are owned by the system, are not counted against users' quotas.
Thank @errose28 join the discuss. As @sodonnel said, To prevent users writing large keys to DDOS the cluster, we still need to calculate usedBytes while allocate block. In this case, we can't treat Open Keys as a system, we need to count it into bukcet's usedBytes in real time. So I think the previous implementation of Ozone quota usedBytes may not need to change.
@sodonnel has some suggestions for us to. For example, when we delete a bucket, all the Open keys that need to be synchronized deleted. Maybe we can use the Open Key Cleanup service to do this.
The problem with writing small files requires at least 3* blocks. I see HDFS also requires at least 3* blocks. So we can be consistent with HDFS and ignore this 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1095357010
Hey thanks for the quick PR and discussions, sorry I am a bit late joining here. @sodonnel definitely raised some valid points. Here is how I was thinking this could work, let me know your thoughts.
- Each allocate block request checks that `existing open key size` + `the block to be allocated` + `current quota usage` <= `max quota`. If this condition fails, allocate block fails.
- Each key commit checks that `total key size` + `current quota usage` <= `max quota`. If so, it updates the quota usage and commits the key.
I think this would address most of the already mentioned problems:
- Open keys, which are owned by the system, are not counted against users' quotas.
- If a large key write exceeds the quota, it fails as soon as possible (when the block pushing it over the quota is allocated), which might be the best we can do in a streaming scenario.
- This also prevents large uncommitted key writes from vastly exceeding the quota.
- Quota usage only reflects committed data.
- Open keys that cumulatively exceed the quota can coexist independently, and only need to acknowledge each other once committed.
- For example:
1. 1GB quota remaining
2. Two 1GB open keys being written: k1 and k2.
- k1 and k2 both successfully allocate all blocks they need and write them. Neither is committed so they have no knowledge of each other.
3. k1 is committed.
4. k2 either calls commit, or attempts to allocate another block. This would cause it to exceed the quota that k1 just filled up, so k2's write fails at this point.
The case @captainzmc mentioned where remaining quota is less than 3X block size would still be a problem with this approach. I am not sure of a good solution for this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r867239448
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Makes sense. We can fix the issue in that PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If possible, I will change 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1109410209
Hi @captainzmc, thanks for updating this PR. I have left a few comments, please check.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1106478371
> Thanks for the updates @captainzmc. Can you please update the corresponding tests as well?
Sure. The new error is related to the EC of the latest merge. I will fix these failed case.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1110498205
Hi @errose28, 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1102877691
@captainzmc are we still planning on continuing this PR? If the approach presented above sounds good, I think it would be better to implement it on its own here instead of mixing it with another PR.
> Each allocate block request checks that existing open key size + the block to be allocated + current quota usage <= max quota. If this condition fails, allocate block fails.
Each key commit checks that total key size + current quota usage <= max quota. If so, it updates the quota usage and commits the key.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1105882238
Thanks for the updates @captainzmc. Can you please update the corresponding tests as well?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858340665
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java:
##########
@@ -226,15 +226,9 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKey),
new CacheValue<>(Optional.absent(), trxnLogIndex));
- long scmBlockSize = ozoneManager.getScmBlockSize();
- int factor = omKeyInfo.getReplicationConfig().getRequiredNodes();
omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
- // Block was pre-requested and UsedBytes updated when createKey and
- // AllocatedBlock. The space occupied by the Key shall be based on
- // the actual Key size, and the total Block size applied before should
- // be subtracted.
- long correctedSpace = omKeyInfo.getReplicatedSize() -
- keyArgs.getKeyLocationsList().size() * scmBlockSize * factor;
+
+ long correctedSpace = omKeyInfo.getReplicatedSize();
omBucketInfo.incrUsedBytes(correctedSpace);
Review Comment:
Oops, if it's a multi-part key, where does usedNamespace get increased?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858713109
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java:
##########
@@ -52,7 +52,7 @@ public class OMKeyCreateResponse extends OmKeyResponse {
public OMKeyCreateResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo omKeyInfo, List<OmKeyInfo> parentKeyInfos,
- long openKeySessionID, @Nonnull OmBucketInfo omBucketInfo) {
+ long openKeySessionID, OmBucketInfo omBucketInfo) {
Review Comment:
Please note there are several cases like this.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If that's right, I will change 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1118068313
Hi @errose28, 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1092003417
> also writing large keys is very unlikely to happen in practice.
I don't agree with that at all. Large keys, small keys - they will all get written.
I agree there are problems with the current approach, especially needing to allocate in full block increments. However quota should stop someone from using more than their allocated space on the cluster. What if I start writing massive keys to effectively DDOS the cluster. I can just write and write beyond my quota TB's of data and never commit my keys. I can start many clients to do this in parallel and fill the entire cluster with garbage data.
In some respects, a failed to write key is using space and it could be argued it should count against the quota. It could be argued either way. However the ability for a user to write huge amounts of data beyond their quota, without the system blocking them in some way is bad I think.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kaijchen commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1095875482
> The case @captainzmc mentioned where remaining quota is less than 3X block size would still be a problem with this approach. I am not sure of a good solution for this.
One solution is to allow block allocation exceed quota by at most 1 block size.
* Each allocate block request checks that `existing open key size` + `current quota usage` <= `max quota`.
* Each key commit checks that `total key size` + `current quota usage` <= `max 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r858706851
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java:
##########
@@ -226,15 +226,9 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
new CacheKey<>(openKey),
new CacheValue<>(Optional.absent(), trxnLogIndex));
- long scmBlockSize = ozoneManager.getScmBlockSize();
- int factor = omKeyInfo.getReplicationConfig().getRequiredNodes();
omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
- // Block was pre-requested and UsedBytes updated when createKey and
- // AllocatedBlock. The space occupied by the Key shall be based on
- // the actual Key size, and the total Block size applied before should
- // be subtracted.
- long correctedSpace = omKeyInfo.getReplicatedSize() -
- keyArgs.getKeyLocationsList().size() * scmBlockSize * factor;
+
+ long correctedSpace = omKeyInfo.getReplicatedSize();
omBucketInfo.incrUsedBytes(correctedSpace);
Review Comment:
> Should check quota and also increase usedNamespace when commit.
> Oops, if it's a multi-part key, where does usedNamespace get increased?
It seems S3MultipartUpload support for Quota has some bugs and lacks some UT. This PR we modified the calculation method of correctedSpace first. And I opened a new JIRA HDDS-6650. I will check and fix all the problems of S3MultipartUpload Quota in the next PR, and add some new 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front.
[This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR.
[This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
@errose28 What do you think of this change? If possible, I will change this in this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5243. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] captainzmc commented on a diff in pull request #3286: HDDS-6556. Check quota while allocate and update usedBytes when commit.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #3286:
URL: https://github.com/apache/ozone/pull/3286#discussion_r866682965
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -196,36 +196,41 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
}
}
+ // If bucket versioning is turned on during the update, between key
+ // creation and key commit, old versions will be just overwritten and
+ // not kept. Bucket versioning will be effective from the first key
+ // creation after the knob turned on.
+ RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
+ OmKeyInfo keyToDelete =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
+ oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
+ keyToDelete, omMetadataManager,
+ trxnLogIndex, ozoneManager.isRatisEnabled());
+ }
+
omKeyInfo =
omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey);
if (omKeyInfo == null) {
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+ omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
omKeyInfo.setDataSize(commitKeyArgs.getDataSize());
-
omKeyInfo.setModificationTime(commitKeyArgs.getModificationTime());
-
// Update the block length for each block
- List<OmKeyLocationInfo> allocatedLocationInfoList =
- omKeyInfo.getLatestVersionLocations().getLocationList();
omKeyInfo.updateLocationInfoList(locationInfoList, false);
-
// Set the UpdateID to current transactionLogIndex
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
- // If bucket versioning is turned on during the update, between key
- // creation and key commit, old versions will be just overwritten and
- // not kept. Bucket versioning will be effective from the first key
- // creation after the knob turned on.
- RepeatedOmKeyInfo oldKeyVersionsToDelete = null;
- OmKeyInfo keyToDelete =
- omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
- if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled()) {
- oldKeyVersionsToDelete = getOldVersionsToCleanUp(dbOzoneKey,
- keyToDelete, omMetadataManager,
- trxnLogIndex, ozoneManager.isRatisEnabled());
+ long correctedSpace = omKeyInfo.getReplicatedSize();
+ // Subtract the size of blocks to be overwritten.
+ if (keyToDelete != null) {
+ correctedSpace -= keyToDelete.getReplicatedSize();
Review Comment:
Thanks @errose28 for start review.
This keyToDelete was added in HDDS-5461. And we didn't change this part in this PR. Just move this code to the front. [This part of the description](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L217) gives us some hints.
I think there's a bug here.
If omBucketInfo.getIsVersionEnabled() is false, each key keep only one version, here we minus the keyToDelete usedBytes is reasonable.
But if omBucketInfo.getIsVersionEnabled() to true, the key should be able to keep multiple versions. And the [keyToDelete will not be deleted](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java#L224). So the keyToDelete's usedBytes should not detracted.
So I think we can change this “if (keyToDelete != null)” to
`if (keyToDelete != null && !omBucketInfo.getIsVersionEnabled())`
I had submit a new PR, and had request @kuenishi and @errose28 to review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #3286: HDDS-6556. Update usedBytes only when commit key.
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3286:
URL: https://github.com/apache/ozone/pull/3286#issuecomment-1101106931
It seems the last problem we have left to solve here is the user writing and abandoning many small open keys to a bucket, exceeding the bucket's intended quota. IMO open keys should not be tied to buckets, both in terms of quota or bucket deletion, since they have not been committed. To me this problem seems outside the scope of bucket or volume quota. I think it would be better solved by user quotas. I think we should wait to solve this issue once we decide to implement user quotas.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org