lokeshj1703 commented on a change in pull request #1561:
URL: https://github.com/apache/ozone/pull/1561#discussion_r521140093
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMBlockProtocolServer.java
##########
@@ -224,54 +224,49 @@ public void join() throws InterruptedException {
}
List<DeleteBlockGroupResult> results = new ArrayList<>();
Map<String, String> auditMap = Maps.newHashMap();
- for (BlockGroup keyBlocks : keyBlocksInfoList) {
- ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result resultCode;
- try {
- // We delete blocks in an atomic operation to prevent getting
- // into state like only a partial of blocks are deleted,
- // which will leave key in an inconsistent state.
- auditMap.put("keyBlockToDelete", keyBlocks.toString());
- scm.getScmBlockManager().deleteBlocks(keyBlocks.getBlockIDList());
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.success;
- AUDIT.logWriteSuccess(
- buildAuditMessageForSuccess(SCMAction.DELETE_KEY_BLOCK, auditMap)
- );
- } catch (SCMException scmEx) {
- LOG.warn("Fail to delete block: {}", keyBlocks.getGroupID(), scmEx);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- scmEx)
- );
- switch (scmEx.getResult()) {
- case SAFE_MODE_EXCEPTION:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.safeMode;
- break;
- case FAILED_TO_FIND_BLOCK:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.errorNotFound;
- break;
- default:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
- }
- } catch (IOException ex) {
- LOG.warn("Fail to delete blocks for object key: {}", keyBlocks
- .getGroupID(), ex);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- ex)
- );
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
+ ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result codeResult;
+ try{
+ scm.getScmBlockManager().deleteBlocks(keyBlocksInfoList);
+ codeResult = ScmBlockLocationProtocolProtos.
+ DeleteScmBlockResult.Result.success;
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
Review comment:
This should be logWriteSuccess.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java
##########
@@ -315,8 +318,8 @@ public void deleteBlocks(List<BlockID> blockIDs) throws
IOException {
} catch (IOException e) {
throw new IOException(
"Skip writing the deleted blocks info to"
- + " the delLog because addTransaction fails. Batch skipped: "
- + StringUtils.join(",", blockIDs), e);
+ + " the delLog because addTransaction fails. Keys skipped: "
+ + keyBlocksInfoList.size(), e);
Review comment:
```suggestion
+ " the delLog because addTransaction fails. " +
keyBlocksInfoList.size() + " Keys skipped", e);
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMBlockProtocolServer.java
##########
@@ -224,54 +224,49 @@ public void join() throws InterruptedException {
}
List<DeleteBlockGroupResult> results = new ArrayList<>();
Map<String, String> auditMap = Maps.newHashMap();
- for (BlockGroup keyBlocks : keyBlocksInfoList) {
- ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result resultCode;
- try {
- // We delete blocks in an atomic operation to prevent getting
- // into state like only a partial of blocks are deleted,
- // which will leave key in an inconsistent state.
- auditMap.put("keyBlockToDelete", keyBlocks.toString());
- scm.getScmBlockManager().deleteBlocks(keyBlocks.getBlockIDList());
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.success;
- AUDIT.logWriteSuccess(
- buildAuditMessageForSuccess(SCMAction.DELETE_KEY_BLOCK, auditMap)
- );
- } catch (SCMException scmEx) {
- LOG.warn("Fail to delete block: {}", keyBlocks.getGroupID(), scmEx);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- scmEx)
- );
- switch (scmEx.getResult()) {
- case SAFE_MODE_EXCEPTION:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.safeMode;
- break;
- case FAILED_TO_FIND_BLOCK:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.errorNotFound;
- break;
- default:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
- }
- } catch (IOException ex) {
- LOG.warn("Fail to delete blocks for object key: {}", keyBlocks
- .getGroupID(), ex);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- ex)
- );
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
+ ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result codeResult;
Review comment:
Can we rename codeResult to just result?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMBlockProtocolServer.java
##########
@@ -224,54 +224,49 @@ public void join() throws InterruptedException {
}
List<DeleteBlockGroupResult> results = new ArrayList<>();
Map<String, String> auditMap = Maps.newHashMap();
- for (BlockGroup keyBlocks : keyBlocksInfoList) {
- ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result resultCode;
- try {
- // We delete blocks in an atomic operation to prevent getting
- // into state like only a partial of blocks are deleted,
- // which will leave key in an inconsistent state.
- auditMap.put("keyBlockToDelete", keyBlocks.toString());
- scm.getScmBlockManager().deleteBlocks(keyBlocks.getBlockIDList());
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.success;
- AUDIT.logWriteSuccess(
- buildAuditMessageForSuccess(SCMAction.DELETE_KEY_BLOCK, auditMap)
- );
- } catch (SCMException scmEx) {
- LOG.warn("Fail to delete block: {}", keyBlocks.getGroupID(), scmEx);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- scmEx)
- );
- switch (scmEx.getResult()) {
- case SAFE_MODE_EXCEPTION:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.safeMode;
- break;
- case FAILED_TO_FIND_BLOCK:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.errorNotFound;
- break;
- default:
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
- }
- } catch (IOException ex) {
- LOG.warn("Fail to delete blocks for object key: {}", keyBlocks
- .getGroupID(), ex);
- AUDIT.logWriteFailure(
- buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
- ex)
- );
- resultCode = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
- .Result.unknownFailure;
+ ScmBlockLocationProtocolProtos.DeleteScmBlockResult.Result codeResult;
+ try{
+ scm.getScmBlockManager().deleteBlocks(keyBlocksInfoList);
+ codeResult = ScmBlockLocationProtocolProtos.
+ DeleteScmBlockResult.Result.success;
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.DELETE_KEY_BLOCK, auditMap));
+ } catch (SCMException scmEx){
+ LOG.warn("Fail to delete {} keys", keyBlocksInfoList.size(), scmEx);
+ AUDIT.logWriteFailure(
+ buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
+ scmEx)
+ );
+ switch (scmEx.getResult()) {
+ case SAFE_MODE_EXCEPTION:
+ codeResult = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
+ .Result.safeMode;
+ break;
+ case FAILED_TO_FIND_BLOCK:
+ codeResult = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
+ .Result.errorNotFound;
+ break;
+ default:
+ codeResult = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
+ .Result.unknownFailure;
}
- List<DeleteBlockResult> blockResultList = new ArrayList<>();
- for (BlockID blockKey : keyBlocks.getBlockIDList()) {
- blockResultList.add(new DeleteBlockResult(blockKey, resultCode));
+ } catch (IOException ex){
+ LOG.warn("Fail to delete {} keys", keyBlocksInfoList.size(), ex);
+ AUDIT.logWriteFailure(
+ buildAuditMessageForFailure(SCMAction.DELETE_KEY_BLOCK, auditMap,
+ ex)
+ );
+ codeResult = ScmBlockLocationProtocolProtos.DeleteScmBlockResult
+ .Result.unknownFailure;
+ }
+ for(BlockGroup bg : keyBlocksInfoList){
+ auditMap.put("KeyBlockToDelete", bg.toString());
Review comment:
auditMap is used in the audit log messages. So we will have to populate
it before the audit log calls.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
