HDDS-3930. Fix OMKeyDeletesRequest. #1169

bharatviswa504 · 2020-07-07T00:38:49Z

What changes were proposed in this pull request?

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

smengcl

Thanks @bharatviswa504 for the PR. I have some comments.

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

codecov-commenter · 2020-07-09T00:51:48Z

Codecov Report

Merging #1169 into master will increase coverage by 0.08%.
The diff coverage is 94.23%.

@@             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	Coverage Δ	Complexity Δ
...pache/hadoop/ozone/om/request/OMClientRequest.java	`86.56% <ø> (-0.78%)`	`26.00 <0.00> (-3.00)`
...doop/ozone/om/request/key/OMKeysDeleteRequest.java	`94.56% <92.10%> (+7.35%)`	`10.00 <0.00> (+1.00)`
...n/java/org/apache/hadoop/ozone/audit/OMAction.java	`100.00% <100.00%> (ø)`	`2.00 <0.00> (ø)`
...op/ozone/om/response/key/OMKeysDeleteResponse.java	`100.00% <100.00%> (+20.51%)`	`4.00 <2.00> (+1.00)`
...doop/ozone/om/exceptions/OMNotLeaderException.java	`48.48% <0.00%> (-18.19%)`	`3.00% <0.00%> (-2.00%)`
...e/container/keyvalue/impl/ChunkManagerFactory.java	`70.00% <0.00%> (-8.58%)`	`4.00% <0.00%> (ø%)`
...p/ozone/security/OzoneDelegationTokenSelector.java	`72.22% <0.00%> (-5.56%)`	`7.00% <0.00%> (-1.00%)`
...PB/OzoneManagerProtocolServerSideTranslatorPB.java	`82.92% <0.00%> (-2.44%)`	`17.00% <0.00%> (-1.00%)`
...iner/ozoneimpl/ContainerScrubberConfiguration.java	`80.00% <0.00%> (-1.82%)`	`8.00% <0.00%> (+1.00%)`	⬇️
...doop/ozone/container/keyvalue/KeyValueHandler.java	`63.55% <0.00%> (-1.56%)`	`66.00% <0.00%> (-1.00%)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a84d5bc...62eded8. Read the comment docs.

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

...zone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

adoroszlai

Thanks @bharatviswa504 for updating the patch. Audit now looks good. There are a few places where I think more change is needed.

adoroszlai · 2020-07-10T17:23:53Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

-      }
-    }
+  public void deleteKeys(OmDeleteKeys deleteKeys) throws IOException {
+    throw new NotImplementedException("OzoneManager does not require this to " +


Please consider changing to UnsupportedOperationException as

NotImplementedException represents the case where the author has yet to implement the logic at this point in the program

adoroszlai · 2020-07-10T17:35:45Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

+      for (int i = indexFailed; i < length; i++) {
+        unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));


Should not repeatedly add the same key:

Suggested change

for (int i = indexFailed; i < length; i++) {

unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));

for (int i = indexFailed; i < length; i++) {

unDeletedKeys.addKeys(deleteKeyArgs.getKeys(i));

adoroszlai · 2020-07-10T17:36:58Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

+
+    auditLog(auditLogger, buildAuditMessage(
+        OMAction.DELETE_KEYS, auditMap, exception, userInfo));
+

    switch (result) {
    case SUCCESS:
      omMetrics.decNumKeys();


Should decrease number of keys by deleteKeys.size().

adoroszlai · 2020-07-10T17:37:44Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

      LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
          bucketName, keyName);


Might want to log all keys:

Suggested change

LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,

bucketName, keyName);

LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,

bucketName, deleteKeys);

xiaoyuyao · 2020-07-10T18:07:47Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

+
+    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);


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

xiaoyuyao · 2020-07-10T18:08:26Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

    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:{}",


Should we reword the error message to differentiate with the single delete.

xiaoyuyao · 2020-07-10T18:09:18Z

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java

-      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: {}",


OMKeyDeleteRequest=> OMKeysDeleteRequest

xiaoyuyao

Thanks @bharatviswa504 for the update. Just few minor comments, otherwise LGTM.

bharatviswa504 · 2020-07-10T20:25:52Z

Thank You @xiaoyuyao and @adoroszlai for the review.
Addressed review comments.

sonarcloud · 2020-07-10T22:05:07Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
4 Code Smells

77.4% Coverage
0.0% Duplication

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

bharatviswa504 · 2020-07-10T22:48:33Z

This is being closed and opened as #1195
As the branch for this accidentally created in apache repo.

smengcl requested changes Jul 8, 2020

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Show resolved Hide resolved

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java Outdated Show resolved Hide resolved

xiaoyuyao reviewed Jul 8, 2020

View reviewed changes

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java Outdated Show resolved Hide resolved

xiaoyuyao reviewed Jul 9, 2020

View reviewed changes

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java Show resolved Hide resolved

xiaoyuyao reviewed Jul 9, 2020

View reviewed changes

.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java Outdated Show resolved Hide resolved

xiaoyuyao reviewed Jul 9, 2020

View reviewed changes

...zone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeysDeleteResponse.java Show resolved Hide resolved

bharatviswa504 mentioned this pull request Jul 9, 2020

HDDS-3903. OzoneRpcClient support batch rename keys. #1150

Merged

bharatviswa504 and others added 13 commits July 9, 2020 14:22

HDDS-3900. Fix OMKeyDeletesRequest.

e0e2547

revert not required

d769ef7

revert not needed

c5b0102

use trxnLogIndex

c9603a3

fix

3972827

fix

22121c9

fix

223a99c

remove unneeded

ae62dd7

add back

5610238

fix review bug

861d184

Add tests

d6daa33

fix sonar issue

70d5f33

fix review comments

ec8068c

bharatviswa504 force-pushed the HDDS-3930 branch from 62eded8 to ec8068c Compare July 10, 2020 00:19

bharatviswa504 added 3 commits July 9, 2020 17:29

rat check

0cf98c9

fix checkstyle

1338787

fix issue in deletekey

21d1cd0

bharatviswa504 requested a review from smengcl July 10, 2020 00:54

bharatviswa504 added 5 commits July 9, 2020 17:56

handle partial delete in response class"

00458a8

fix TestResultCodes"

c30a77e

revert imports

ace6295

fix checkstyle

ac098b0

remove intellij optimize import changes

cba00cd

bharatviswa504 added 3 commits July 9, 2020 18:55

import change

d1e8c9d

remove unchanged file

204aac7

cs

fb3c794

bharatviswa504 requested review from xiaoyuyao and adoroszlai July 10, 2020 01:59

xiaoyuyao reviewed Jul 10, 2020

View reviewed changes

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Show resolved Hide resolved

bharatviswa504 added 2 commits July 9, 2020 21:14

fix tests

eb7c8df

fix cs

46c7763

bharatviswa504 added om 0.6.0 labels Jul 10, 2020

adoroszlai requested changes Jul 10, 2020

View reviewed changes

xiaoyuyao reviewed Jul 10, 2020

View reviewed changes

address review comments

8c6b465

bharatviswa504 added 2 commits July 10, 2020 13:30

fix metric mistake

a548df2

fix cs

b46762e

bharatviswa504 closed this Jul 10, 2020

bharatviswa504 deleted the HDDS-3930 branch July 10, 2020 22:38

bharatviswa504 restored the HDDS-3930 branch July 10, 2020 22:39

bharatviswa504 reopened this Jul 10, 2020

bharatviswa504 closed this Jul 10, 2020

bharatviswa504 deleted the HDDS-3930 branch July 10, 2020 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-3930. Fix OMKeyDeletesRequest. #1169

HDDS-3930. Fix OMKeyDeletesRequest. #1169

bharatviswa504 commented Jul 7, 2020

smengcl left a comment

codecov-commenter commented Jul 9, 2020 •

edited

adoroszlai left a comment

adoroszlai Jul 10, 2020

adoroszlai Jul 10, 2020

adoroszlai Jul 10, 2020

adoroszlai Jul 10, 2020

xiaoyuyao Jul 10, 2020

xiaoyuyao Jul 10, 2020

xiaoyuyao Jul 10, 2020

xiaoyuyao left a comment

bharatviswa504 commented Jul 10, 2020

sonarcloud bot commented Jul 10, 2020

bharatviswa504 commented Jul 10, 2020

		for (int i = indexFailed; i < length; i++) {
		unDeletedKeys.addKeys(deleteKeyArgs.getKeys(indexFailed));

		LOG.debug("Key deleted. Volume:{}, Bucket:{}, Key:{}", volumeName,
		bucketName, keyName);

HDDS-3930. Fix OMKeyDeletesRequest. #1169

HDDS-3930. Fix OMKeyDeletesRequest. #1169

Conversation

bharatviswa504 commented Jul 7, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

smengcl left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 9, 2020 • edited

Codecov Report

adoroszlai left a comment

Choose a reason for hiding this comment

adoroszlai Jul 10, 2020

Choose a reason for hiding this comment

adoroszlai Jul 10, 2020

Choose a reason for hiding this comment

adoroszlai Jul 10, 2020

Choose a reason for hiding this comment

adoroszlai Jul 10, 2020

Choose a reason for hiding this comment

xiaoyuyao Jul 10, 2020

Choose a reason for hiding this comment

xiaoyuyao Jul 10, 2020

Choose a reason for hiding this comment

xiaoyuyao Jul 10, 2020

Choose a reason for hiding this comment

xiaoyuyao left a comment

Choose a reason for hiding this comment

bharatviswa504 commented Jul 10, 2020

sonarcloud bot commented Jul 10, 2020

bharatviswa504 commented Jul 10, 2020

codecov-commenter commented Jul 9, 2020 •

edited