You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/07/07 00:39:04 UTC
[GitHub] [hadoop-ozone] bharatviswa504 opened a new pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
bharatviswa504 opened a new pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169
## What changes were proposed in this pull request?
1. The cache of the key should be updated in ValidateAndUpdateCache, as we return response once after adding to cache, and before DoubleBuffer flushes to disk using OmClientResponse#addToDBBatch.
2. Changed return type of DeleteKeysResponse, as anyway when success we are not returning delete list and also when failure, we fail complete batch. So we can just return status.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3930
## How was this patch tested?
Existing tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452611714
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -848,7 +850,18 @@ message DeleteKeyRequest {
}
message DeleteKeysRequest {
- repeated KeyArgs keyArgs = 1;
+ optional DeleteKeyArgs deleteKeys = 1;
Review comment:
Should this be required?
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -848,7 +850,18 @@ message DeleteKeyRequest {
}
message DeleteKeysRequest {
- repeated KeyArgs keyArgs = 1;
+ optional DeleteKeyArgs deleteKeys = 1;
Review comment:
Should this be required instead of optional?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] removed a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831930
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='95.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [95.0% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451906535
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +111,116 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(indexFailed);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
Review comment:
The Acl check could also failed delete for some files, this could also be reflected in the undeleted list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451940527
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
The current behavior is we fail the entire batch if we are not able to delete the key.
So, proposal is to change this behavior?
And also I see we fail to delete only when key not found. In this case, there is no real point in returning them as unDeletedKeys. Not sure, even in this case if it is helpful.
2nd case we fail is when checking ACLS, when user does not have permission, in this case even if we return unDeletedKeys, not sure how it will be used.
3rd case, when reading DB failure, then also whole batch will be failed. As anyway, at that time OM will be terminated, as this is a critical error to avoid DB divergence.
So, still, I am not really sure what is real use case of returning unDeletedKeys and how that will be used. Just trying to understand it.
The current PR, kept the behavior of existing behavior,(but only removed return of unDeletedKeys) where it used to fail the entire batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451283029
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
I don't see any usage of it, and also I see no real usage of this result in the client.
And this is a new API added in current trunk, and will be released in the next ozone release.
One more reason, as we fail whole batch, I don't see real use case for it as of current behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451666766
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
Review comment:
yes, it is a typo. Not sure why any test has caught this bug.
I will add some UT's to test 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451940527
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
The current behavior is we fail the entire batch if we are not able to delete the key.
So, proposal is to change this behavior?
And also I see we fail to delete only when key not found. In this case, there is no real point in returning them as unDeletedKeys.
2nd case we fail is when checking ACLS, when user does not have permission, in this case even if we return unDeletedKeys, not sure how it will be used.
3rd case, when reading DB failure, then also whole batch will be failed. As anyway, at that time OM will be terminated, as this is a critical error to avoid DB divergence.
So, still, I am not really sure what is real use case of returning unDeletedKeys and how that will be used. Just trying to understand it.
The current PR, kept the behavior of existing behavior,(but only removed return of unDeletedKeys) where it used to fail the entire batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] removed a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655671035
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451906339
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +111,116 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(indexFailed);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
Review comment:
Client may get an outdated list of keys and send the list for delete. Instead of fail the complete deletion, I suggest we update the undelete list that returned to the client.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452612030
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Looks good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452098749
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
-
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
-
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
+ omKeyInfoList.add(omKeyInfo);
+ }
- if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
- }
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
- }
+ // Mark all keys in cache as deleted.
+ for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ keyName = deleteKeyArgs.getKeyName();
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName)),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
}
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(true)).build(), omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
- }
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
+ // When we get any error during iteration build the remaining audit map
+ // from deleteKeyArgsList.
+ for (int i = indexFailed; i < deleteKeyArgsList.size(); i++) {
+ buildKeyArgsAuditMap(deleteKeyArgsList.get(i));
}
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
Review comment:
Sorry, but I don't think it's fixed. `auditMap` still contains info about only one key.
It is updated for each key in the first loop, overwriting previous values:
https://github.com/apache/hadoop-ozone/blob/62eded87415377b617d69b195f519b7fcb594e40/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java#L139-L142
as the map is not used until this `auditLog()` call.
Furthermore, audit map is built but discarded (not even stored in local var) for keys starting with `indexFailed`:
https://github.com/apache/hadoop-ozone/blob/62eded87415377b617d69b195f519b7fcb594e40/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java#L204-L207
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452996069
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
}
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).setUnDeletedKeys(unDeletedKeys).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
+
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
- }
+ auditMap = buildDeleteKeysAuditMap(volumeName, bucketName, deleteKeys,
+ unDeletedKeys.getKeysList());
+
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+
switch (result) {
case SUCCESS:
omMetrics.decNumKeys();
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
bucketName, keyName);
break;
- case REPLAY:
- LOG.debug("Replayed Transaction {} ignored. Request: {}",
- trxnLogIndex, deleteKeyRequest);
- break;
case FAILURE:
omMetrics.incNumKeyDeleteFails();
- LOG.error("Key delete failed. Volume:{}, Bucket:{}, Key{}." +
- " Exception:{}", volumeName, bucketName, keyName, exception);
+ LOG.error("Key delete failed. Volume:{}, Bucket:{}, Key:{}",
Review comment:
Should we reword the error message to differentiate with the single delete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831566
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451283029
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
I don't see any usage of it, and also I see no real usage of this result in the client.
And this is a new API adding, and will be released in next ozone release.
One more reason, as we fail whole batch, I don't see real use case for it as of current behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] removed a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-656912925
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='77.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [77.1% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452974904
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2220,16 +2224,13 @@ public void deleteKey(OmKeyArgs args) throws IOException {
/**
* Deletes an existing key.
*
- * @param args - List attributes of the key.
+ * @param deleteKeys - List of keys to be deleted from volume and a bucket.
* @throws IOException
*/
@Override
- public void deleteKeys(List<OmKeyArgs> args) throws IOException {
- if (args != null) {
- for (OmKeyArgs keyArgs : args) {
- deleteKey(keyArgs);
- }
- }
+ public void deleteKeys(OmDeleteKeys deleteKeys) throws IOException {
+ throw new NotImplementedException("OzoneManager does not require this to " +
Review comment:
Please consider changing to `UnsupportedOperationException` as
> [`NotImplementedException`](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/NotImplementedException.html) represents the case where the author has yet to implement the logic at this point in the program
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
}
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).setUnDeletedKeys(unDeletedKeys).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
+
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
- }
+ auditMap = buildDeleteKeysAuditMap(volumeName, bucketName, deleteKeys,
+ unDeletedKeys.getKeysList());
+
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+
switch (result) {
case SUCCESS:
omMetrics.decNumKeys();
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
bucketName, keyName);
Review comment:
Might want to log all keys:
```suggestion
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
bucketName, deleteKeys);
```
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
}
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).setUnDeletedKeys(unDeletedKeys).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
+
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
- }
+ auditMap = buildDeleteKeysAuditMap(volumeName, bucketName, deleteKeys,
+ unDeletedKeys.getKeysList());
+
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+
switch (result) {
case SUCCESS:
omMetrics.decNumKeys();
Review comment:
Should decrease number of keys by `deleteKeys.size()`.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
Review comment:
Should not repeatedly add the same key:
```suggestion
for (int i = indexFailed; i < length; i++) {
unDeletedKeys.addKeys(deleteKeyArgs.getKeys(i));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] codecov-commenter edited a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831767
# [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=h1) Report
> Merging [#1169](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hadoop-ozone/commit/a84d5bc425c9bc2b1fcfd9c7c88e160c22fef253&el=desc) will **increase** coverage by `0.08%`.
> The diff coverage is `94.23%`.
[](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1169 +/- ##
============================================
+ Coverage 73.50% 73.59% +0.08%
- Complexity 10037 10137 +100
============================================
Files 974 974
Lines 49685 50086 +401
Branches 4883 4979 +96
============================================
+ Hits 36523 36862 +339
- Misses 10845 10889 +44
- Partials 2317 2335 +18
```
| [Impacted Files](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pache/hadoop/ozone/om/request/OMClientRequest.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXF1ZXN0L09NQ2xpZW50UmVxdWVzdC5qYXZh) | `86.56% <ø> (-0.78%)` | `26.00 <0.00> (-3.00)` | |
| [...doop/ozone/om/request/key/OMKeysDeleteRequest.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXF1ZXN0L2tleS9PTUtleXNEZWxldGVSZXF1ZXN0LmphdmE=) | `94.56% <92.10%> (+7.35%)` | `10.00 <0.00> (+1.00)` | |
| [...n/java/org/apache/hadoop/ozone/audit/OMAction.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL2F1ZGl0L09NQWN0aW9uLmphdmE=) | `100.00% <100.00%> (ø)` | `2.00 <0.00> (ø)` | |
| [...op/ozone/om/response/key/OMKeysDeleteResponse.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXNwb25zZS9rZXkvT01LZXlzRGVsZXRlUmVzcG9uc2UuamF2YQ==) | `100.00% <100.00%> (+20.51%)` | `4.00 <2.00> (+1.00)` | |
| [...doop/ozone/om/exceptions/OMNotLeaderException.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL29tL2V4Y2VwdGlvbnMvT01Ob3RMZWFkZXJFeGNlcHRpb24uamF2YQ==) | `48.48% <0.00%> (-18.19%)` | `3.00% <0.00%> (-2.00%)` | |
| [...e/container/keyvalue/impl/ChunkManagerFactory.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvaW1wbC9DaHVua01hbmFnZXJGYWN0b3J5LmphdmE=) | `70.00% <0.00%> (-8.58%)` | `4.00% <0.00%> (ø%)` | |
| [...p/ozone/security/OzoneDelegationTokenSelector.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL3NlY3VyaXR5L096b25lRGVsZWdhdGlvblRva2VuU2VsZWN0b3IuamF2YQ==) | `72.22% <0.00%> (-5.56%)` | `7.00% <0.00%> (-1.00%)` | |
| [...PB/OzoneManagerProtocolServerSideTranslatorPB.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9wcm90b2NvbFBCL096b25lTWFuYWdlclByb3RvY29sU2VydmVyU2lkZVRyYW5zbGF0b3JQQi5qYXZh) | `82.92% <0.00%> (-2.44%)` | `17.00% <0.00%> (-1.00%)` | |
| [...iner/ozoneimpl/ContainerScrubberConfiguration.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIvb3pvbmVpbXBsL0NvbnRhaW5lclNjcnViYmVyQ29uZmlndXJhdGlvbi5qYXZh) | `80.00% <0.00%> (-1.82%)` | `8.00% <0.00%> (+1.00%)` | :arrow_down: |
| [...doop/ozone/container/keyvalue/KeyValueHandler.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvS2V5VmFsdWVIYW5kbGVyLmphdmE=) | `63.55% <0.00%> (-1.56%)` | `66.00% <0.00%> (-1.00%)` | |
| ... and [16 more](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=footer). Last update [a84d5bc...62eded8](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451907170
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java
##########
@@ -69,65 +64,36 @@ public OMKeysDeleteResponse(@Nonnull OMResponse omResponse) {
public void addToDBBatch(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
+ String volumeName = "";
+ String bucketName = "";
+ String keyName = "";
for (OmKeyInfo omKeyInfo : omKeyInfoList) {
Review comment:
we should add only those that can be deleted (pass existence check and acl check, etc.) to the batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452613394
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -848,7 +850,18 @@ message DeleteKeyRequest {
}
message DeleteKeysRequest {
- repeated KeyArgs keyArgs = 1;
+ optional DeleteKeyArgs deleteKeys = 1;
Review comment:
In proto 3 onwards all fields are optional.
So, followed that approach and declared optional. (In future if something is changed, we can still be backward compatible)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451240368
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
`unDeletedKeys` seems to be added in this [comment](https://github.com/apache/hadoop-ozone/pull/814/files#r429342829) on purpose.
Will removing those two fields cause any compatibility issue?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
Review comment:
Why do we always get the first element here? Typo?
Also, I didn't find existing test for `OMKeysDeleteRequest` or `OMKeysDeleteResponse`. It'd be a good idea to add some tests for sanity checks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452996554
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
}
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).setUnDeletedKeys(unDeletedKeys).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
+
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
- }
+ auditMap = buildDeleteKeysAuditMap(volumeName, bucketName, deleteKeys,
+ unDeletedKeys.getKeysList());
+
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+
switch (result) {
case SUCCESS:
omMetrics.decNumKeys();
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
bucketName, keyName);
break;
- case REPLAY:
- LOG.debug("Replayed Transaction {} ignored. Request: {}",
- trxnLogIndex, deleteKeyRequest);
- break;
case FAILURE:
omMetrics.incNumKeyDeleteFails();
- LOG.error("Key delete failed. Volume:{}, Bucket:{}, Key{}." +
- " Exception:{}", volumeName, bucketName, keyName, exception);
+ LOG.error("Key delete failed. Volume:{}, Bucket:{}, Key:{}",
+ volumeName, bucketName, keyName, exception);
break;
default:
LOG.error("Unrecognized Result for OMKeyDeleteRequest: {}",
Review comment:
OMKeyDeleteRequest=> OMKeysDeleteRequest
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451859191
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Added tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451797541
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Thanks. Makes sense to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452985654
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -848,7 +850,18 @@ message DeleteKeyRequest {
}
message DeleteKeysRequest {
- repeated KeyArgs keyArgs = 1;
+ optional DeleteKeyArgs deleteKeys = 1;
Review comment:
Good to know. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452560417
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Created https://issues.apache.org/jira/browse/HDDS-3948 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451940527
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
The current behavior is we fail the entire batch if we are not able to delete the key.
So, proposal is to change this behavior?
And also I see we fail to delete only when key not found. In this case, there is no real point in returning them as unDeletedKeys. Not sure, even in this case if it is helpful.
2nd case we fail is when checking ACLS, when user does not have permission, in this case even if we return unDeletedKeys, not sure how it will be used.
3rd case, when reading DB failure, then also whole batch will be failed. As anyway, at that time OM will be terminated, as this is a critical error to avoid DB divergence.
So, still, I am not really sure what is real use case of returning unDeletedKeys and how that will be used. Just trying to understand it can be used.
The current PR, kept the behavior of existing behavior,(but only removed return of unDeletedKeys) where it used to fail the entire batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452613394
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -848,7 +850,18 @@ message DeleteKeyRequest {
}
message DeleteKeysRequest {
- repeated KeyArgs keyArgs = 1;
+ optional DeleteKeyArgs deleteKeys = 1;
Review comment:
From proto 3 onwards all fields are optional.
So, followed that approach and declared optional. (In future if something is changed, we can still be backward compatible)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-656927424
This is being closed and opened as #1195
As the branch for this accidentally created in apache repo.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452474846
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
bq. The current behavior is we fail the entire batch if we are not able to delete the key. So, proposal is to change this behavior?
The goal is to not fail the whole deletion when something failed in the middle and return the list of deleted and undeleted to the client. We can't get there if delete and undelete list are removed from the proto. Here is the reason for keeping them for HDDS-3916.
1. It can be expensive to lookup or retry the whole list again if one of the deletion failed.
2. Returning undelete file list reduce the amount of unnecessary processing.
Also holding the bucket lock once for all may not be a good idea to large delete as that will prevent other clients operation on the same bucket. grab and release individual or in small batches give other clients opportunities to access buckets.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452559759
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
As in HA, and when ratis is enabled as default for non-HA, it is a single thread executor now. So, this optimization is not really required. For non-HA, it might help, I will open a new Jira for this improvement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452995758
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +98,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
- keyName);
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
- // check Acl
- checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
- IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ int indexFailed = 0;
+ int length = deleteKeys.size();
+ OzoneManagerProtocolProtos.DeleteKeyArgs.Builder unDeletedKeys =
+ OzoneManagerProtocolProtos.DeleteKeyArgs.newBuilder()
+ .setVolumeName(volumeName).setBucketName(bucketName);
+
+ boolean deleteStatus = true;
+ try {
+ // Validate bucket and volume exists or not.
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ for (indexFailed = 0; indexFailed < length; indexFailed++) {
+ keyName = deleteKeyArgs.getKeys(indexFailed);
+ String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
+ deleteStatus = false;
+ LOG.error("Received a request to delete a Key does not exist {}",
+ objectKey);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
+ continue;
}
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
+ try {
+ // check Acl
+ checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ omKeyInfoList.add(omKeyInfo);
+ } catch (Exception ex) {
+ deleteStatus = false;
+ LOG.error("Acl check failed for Key: {}", objectKey, ex);
+ deleteKeys.remove(keyName);
+ unDeletedKeys.addKeys(keyName);
}
}
+ // Mark all keys which can be deleted, in cache as deleted.
+ for (OmKeyInfo omKeyInfo : omKeyInfoList) {
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ omKeyInfo.getKeyName())),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
+ }
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
+ .setStatus(deleteStatus ? OK : PARTIAL_DELETE)
+ .setSuccess(deleteStatus).build(),
+ omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
+
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+
+ // reset deleteKeys as request failed.
+ deleteKeys = new ArrayList<>();
+ // Add all keys which are failed due to any other exception .
+ for (int i = indexFailed; i < length; i++) {
+ unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));
}
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).setUnDeletedKeys(unDeletedKeys).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
+
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
- }
+ auditMap = buildDeleteKeysAuditMap(volumeName, bucketName, deleteKeys,
+ unDeletedKeys.getKeysList());
+
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+
switch (result) {
case SUCCESS:
omMetrics.decNumKeys();
LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
bucketName, keyName);
Review comment:
In the success case, the omMetrics.decNumKeys() only -1, which does not reflect the deleted keys if there are more than one key in the request.
NIT: in success case, we only put the keyName of the last successful delete. If we want to put the names of all deleted keys. We should guard it with if (LOG.isDebugEnabled())
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451675182
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Will restore the original behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655671035
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451962232
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +111,116 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(indexFailed);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
Review comment:
Yes agree with this part, will change it to return batch delete with ignoring KEY_NOT_FOUND,
But I still I don't get what is the purpose of returning this as unDeletedKeys, do we want to send a status also for each Key then?
Like Key1, KEY_NOT_FOUND
Key2, ACL_ERROR
What is the plan/how this will be used??
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451662992
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
-
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
-
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
+ omKeyInfoList.add(omKeyInfo);
+ }
- if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
- }
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
- }
+ // Mark all keys in cache as deleted.
+ for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ keyName = deleteKeyArgs.getKeyName();
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName)),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
}
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(true)).build(), omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
- }
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
+ // When we get any error during iteration build the remaining audit map
+ // from deleteKeyArgsList.
+ for (int i = indexFailed; i < deleteKeyArgsList.size(); i++) {
+ buildKeyArgsAuditMap(deleteKeyArgsList.get(i));
}
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
Review comment:
As @smengcl pointed out, we only audit the first delete and miss all the others here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451940527
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
The current behavior is we fail the entire batch if we are not able to delete the key.
So, proposal is to change this behavior?
And also I see we fail to delete only when key not found. In this case, there is no real point in returning them as unDeletedKeys.
2nd case we fail is when checking ACLS, when user does not have permission, in this case even if we return unDeletedKeys, not sure how it will be used.
3rd case, when reading DB failure, then also whole batch will be failed. As anyway, at that time OM will be terminated, as this is a critical error to avoid DB divergence.
So, still, I am not really sure what is real use case of returning unDeletedKeys and how that will be used. Just trying to understand it.
The current PR, kept the behavior of existing behavior, where it used to fail entire batch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452484762
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
Got it. So, when this information is returned if the plan is to retry, I will add them.
The reason for my question is, we fail when we don't have permission/key not found we return them, but if this information is useful, will add that back.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 closed pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 closed pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] codecov-commenter commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831767
# [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=h1) Report
> Merging [#1169](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hadoop-ozone/commit/a84d5bc425c9bc2b1fcfd9c7c88e160c22fef253&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `94.23%`.
[](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1169 +/- ##
============================================
- Coverage 73.50% 73.41% -0.10%
+ Complexity 10037 10021 -16
============================================
Files 974 974
Lines 49685 49695 +10
Branches 4883 4888 +5
============================================
- Hits 36523 36483 -40
- Misses 10845 10883 +38
- Partials 2317 2329 +12
```
| [Impacted Files](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pache/hadoop/ozone/om/request/OMClientRequest.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXF1ZXN0L09NQ2xpZW50UmVxdWVzdC5qYXZh) | `86.56% <ø> (-0.78%)` | `26.00 <0.00> (-3.00)` | |
| [...doop/ozone/om/request/key/OMKeysDeleteRequest.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXF1ZXN0L2tleS9PTUtleXNEZWxldGVSZXF1ZXN0LmphdmE=) | `94.56% <92.10%> (+7.35%)` | `10.00 <0.00> (+1.00)` | |
| [...n/java/org/apache/hadoop/ozone/audit/OMAction.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL2F1ZGl0L09NQWN0aW9uLmphdmE=) | `100.00% <100.00%> (ø)` | `2.00 <0.00> (ø)` | |
| [...op/ozone/om/response/key/OMKeysDeleteResponse.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL296b25lLW1hbmFnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9vbS9yZXNwb25zZS9rZXkvT01LZXlzRGVsZXRlUmVzcG9uc2UuamF2YQ==) | `100.00% <100.00%> (+20.51%)` | `4.00 <2.00> (+1.00)` | |
| [...hdds/scm/container/common/helpers/ExcludeList.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy9zY20vY29udGFpbmVyL2NvbW1vbi9oZWxwZXJzL0V4Y2x1ZGVMaXN0LmphdmE=) | `78.26% <0.00%> (-21.74%)` | `17.00% <0.00%> (-5.00%)` | |
| [...doop/ozone/om/exceptions/OMNotLeaderException.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL29tL2V4Y2VwdGlvbnMvT01Ob3RMZWFkZXJFeGNlcHRpb24uamF2YQ==) | `48.48% <0.00%> (-18.19%)` | `3.00% <0.00%> (-2.00%)` | |
| [...p/ozone/security/OzoneDelegationTokenSelector.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLW96b25lL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL296b25lL3NlY3VyaXR5L096b25lRGVsZWdhdGlvblRva2VuU2VsZWN0b3IuamF2YQ==) | `72.22% <0.00%> (-5.56%)` | `7.00% <0.00%> (-1.00%)` | |
| [...apache/hadoop/hdds/scm/block/BlockManagerImpl.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvc2VydmVyLXNjbS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hkZHMvc2NtL2Jsb2NrL0Jsb2NrTWFuYWdlckltcGwuamF2YQ==) | `68.46% <0.00%> (-5.41%)` | `19.00% <0.00%> (ø%)` | |
| [...hadoop/hdds/security/token/BlockTokenVerifier.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvZnJhbWV3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy9zZWN1cml0eS90b2tlbi9CbG9ja1Rva2VuVmVyaWZpZXIuamF2YQ==) | `53.33% <0.00%> (-4.45%)` | `6.00% <0.00%> (-1.00%)` | |
| [...iner/common/statemachine/SCMConnectionManager.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIvY29tbW9uL3N0YXRlbWFjaGluZS9TQ01Db25uZWN0aW9uTWFuYWdlci5qYXZh) | `77.38% <0.00%> (-3.58%)` | `13.00% <0.00%> (-1.00%)` | |
| ... and [22 more](https://codecov.io/gh/apache/hadoop-ozone/pull/1169/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=footer). Last update [a84d5bc...62eded8](https://codecov.io/gh/apache/hadoop-ozone/pull/1169?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-656912925
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='77.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [77.1% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451675182
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
As said, if we fail entire batch, not sure what is the purpose of unDeletedKeys.
This will be required if we delete a few entries and skip them. Let me know if i am missing something here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452561797
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +111,116 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(indexFailed);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
Review comment:
Done. For now, made it simple returning unDeletedKeys.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-656874561
Thank You @xiaoyuyao and @adoroszlai for the review.
Addressed review comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451671582
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
HDDS-3916 is in progress to use it on client side to resume delete failures.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655133377
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='96.4%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [96.4% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451907564
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -867,10 +867,10 @@ message DeletedKeys {
}
message DeleteKeysResponse {
- repeated KeyInfo deletedKeys = 1;
- repeated KeyInfo unDeletedKeys = 2;
Review comment:
bq. One more reason, as we fail whole batch, I don't see real use case for it as of current behavior.
I think the writebatch should contain only those that can be deleted. Those undeleted will be returned to client.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] removed a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655133377
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='96.4%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [96.4% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452561712
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
-
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
-
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
+ omKeyInfoList.add(omKeyInfo);
+ }
- if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
- }
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
- }
+ // Mark all keys in cache as deleted.
+ for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ keyName = deleteKeyArgs.getKeyName();
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName)),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
}
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(true)).build(), omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
- }
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
+ // When we get any error during iteration build the remaining audit map
+ // from deleteKeyArgsList.
+ for (int i = indexFailed; i < deleteKeyArgsList.size(); i++) {
+ buildKeyArgsAuditMap(deleteKeyArgsList.get(i));
}
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
Review comment:
Good catch. Thanks fixed it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] removed a comment on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831566
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r451672682
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,89 +111,112 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(0);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
- String objectKey = omMetadataManager.getOzoneKey(
- volumeName, bucketName, keyName);
-
- // Validate bucket and volume exists or not.
- validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
-
- OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
+ omKeyInfoList.add(omKeyInfo);
+ }
- if (omKeyInfo == null) {
- throw new OMException("Key not found: " + keyName, KEY_NOT_FOUND);
- }
- // Check if this transaction is a replay of ratis logs.
- if (isReplay(ozoneManager, omKeyInfo, trxnLogIndex)) {
- // Replay implies the response has already been returned to
- // the client. So take no further action and return a dummy
- // OMClientResponse.
- throw new OMReplayException();
- }
+ // Mark all keys in cache as deleted.
+ for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ keyName = deleteKeyArgs.getKeyName();
+ omMetadataManager.getKeyTable().addCacheEntry(
+ new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName,
+ keyName)),
+ new CacheValue<>(Optional.absent(), trxnLogIndex));
}
+
omClientResponse = new OMKeysDeleteResponse(omResponse
- .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()).build(),
- omKeyInfoList, trxnLogIndex, ozoneManager.isRatisEnabled());
+ .setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(true)).build(), omKeyInfoList, trxnLogIndex,
+ ozoneManager.isRatisEnabled());
result = Result.SUCCESS;
+
} catch (IOException ex) {
- if (ex instanceof OMReplayException) {
- result = Result.REPLAY;
- omClientResponse = new OMKeyDeleteResponse(createReplayOMResponse(
- omResponse));
- } else {
- result = Result.FAILURE;
- exception = ex;
-
- omClientResponse = new OMKeyDeleteResponse(
- createOperationKeysErrorOMResponse(omResponse, exception,
- unDeletedKeys));
- }
+ result = Result.FAILURE;
+ exception = ex;
+ createErrorOMResponse(omResponse, ex);
+ omResponse.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
+ .setStatus(false).build()).build();
+ omClientResponse = new OMKeysDeleteResponse(omResponse.build());
} finally {
+ if (acquiredLock) {
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+ }
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
}
- // Performing audit logging outside of the lock.
- if (result != Result.REPLAY) {
- auditLog(auditLogger, buildAuditMessage(
- OMAction.DELETE_KEY, auditMap, exception, userInfo));
+ // When we get any error during iteration build the remaining audit map
+ // from deleteKeyArgsList.
+ for (int i = indexFailed; i < deleteKeyArgsList.size(); i++) {
+ buildKeyArgsAuditMap(deleteKeyArgsList.get(i));
}
+ auditLog(auditLogger, buildAuditMessage(
+ OMAction.DELETE_KEYS, auditMap, exception, userInfo));
Review comment:
Fixed above issue, that will take care of this issue 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-655831930
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E.png' alt='E' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='95.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [95.0% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#discussion_r452561831
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -116,93 +111,116 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+ // As right now, only client exposed API is for a single volume and
+ // bucket. So, all entries will have same volume name and bucket name.
+ // So, we can validate once.
+ if (deleteKeyArgsList.size() > 0) {
+ volumeName = deleteKeyArgsList.get(0).getVolumeName();
+ bucketName = deleteKeyArgsList.get(0).getBucketName();
+ }
+
+ boolean acquiredLock =
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
+ bucketName);
+
+ int indexFailed = 0;
try {
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
+
+ // Validate bucket and volume exists or not.
+ if (deleteKeyArgsList.size() > 0) {
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ }
+
+
+ // Check if any of the key in the batch cannot be deleted. If exists the
+ // batch delete will be failed.
+
+ for (indexFailed = 0; indexFailed < deleteKeyArgsList.size();
+ indexFailed++) {
+ KeyArgs deleteKeyArgs = deleteKeyArgsList.get(indexFailed);
+ auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
volumeName = deleteKeyArgs.getVolumeName();
bucketName = deleteKeyArgs.getBucketName();
keyName = deleteKeyArgs.getKeyName();
String objectKey = omMetadataManager.getOzoneKey(volumeName, bucketName,
keyName);
OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey);
- omKeyInfoList.add(omKeyInfo);
- unDeletedKeys.add(omKeyInfo);
- }
- // Check if any of the key in the batch cannot be deleted. If exists the
- // batch will delete failed.
- for (KeyArgs deleteKeyArgs : deleteKeyArgsList) {
- volumeName = deleteKeyArgs.getVolumeName();
- bucketName = deleteKeyArgs.getBucketName();
- keyName = deleteKeyArgs.getKeyName();
- auditMap = buildKeyArgsAuditMap(deleteKeyArgs);
+
+ // Do we need to fail the batch if one of the key does not exist?
+ // For now following the previous code behavior. If this code changes
+ // behavior, this will be incompatible change across upgrades, and we
+ // need to version the Requests and do logic accordingly.
+
+ if (omKeyInfo == null) {
+ LOG.error("Key does not exist {}", objectKey);
+ throw new OMException("Key Not Found " + objectKey, KEY_NOT_FOUND);
+ }
+
// check Acl
checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
Review comment:
Done
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java
##########
@@ -69,65 +64,36 @@ public OMKeysDeleteResponse(@Nonnull OMResponse omResponse) {
public void addToDBBatch(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException {
+ String volumeName = "";
+ String bucketName = "";
+ String keyName = "";
for (OmKeyInfo omKeyInfo : omKeyInfoList) {
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 closed pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 closed pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] sonarcloud[bot] commented on pull request #1169: HDDS-3930. Fix OMKeyDeletesRequest.
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #1169:
URL: https://github.com/apache/hadoop-ozone/pull/1169#issuecomment-656913594
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=hadoop-ozone&pullRequest=1169&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='77.4%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list) [77.4% Coverage](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=hadoop-ozone&pullRequest=1169&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_232) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org