[GitHub] [ozone] szetszwo commented on a diff in pull request #4403: HDDS-8155. SCM Block deleting service add transaction blocks for stale/dead/decommissioning DNs

["szetszwo (via GitHub)" <gi...@apache.org>]

szetszwo commented on code in PR #4403:
URL: https://github.com/apache/ozone/pull/4403#discussion_r1144595675


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLog.java:
##########
@@ -43,11 +44,14 @@ public interface DeletedBlockLog extends Closeable {
    * Scan entire log once and returns TXs to DatanodeDeletedBlockTransactions.
    * Once DatanodeDeletedBlockTransactions is full, the scan behavior will
    * stop.
+   *
    * @param blockDeletionLimit Maximum number of blocks to fetch
+   * @param dnUUIDMap map of uuid and DN details

Review Comment:
   From the type, we already know that it is a map of uuid and DN details.  We should talk about what this is for.  BTW, I suggest to change it to `Set` and rename it to `included` as mentioned in another comment.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java:
##########
@@ -134,14 +135,19 @@ public EmptyTaskResult call() throws Exception {
       if (LOG.isDebugEnabled()) {
         LOG.debug("Running DeletedBlockTransactionScanner");
       }
-      // TODO - DECOMM - should we be deleting blocks from decom nodes
-      //        and what about entering maintenance.
       List<DatanodeDetails> datanodes =
           nodeManager.getNodes(NodeStatus.inServiceHealthy());
       if (datanodes != null) {
+        // When DN node is healthy and in-service, and previous commands 
+        // are handled for deleteBlocks Type, then it will be considered
+        // in this iteration
+        Map<UUID, DatanodeDetails> dnUUIDMap = datanodes.stream().filter(

Review Comment:
   Let's declare it as `Set<DatanodeDetails>` and rename it to `included`, i.e.
   ```java
           final Set<DatanodeDetails> included = datanodes.stream()
               .filter(dn -> nodeManager.getCommandQueueCount(
                   dn.getUuid(), Type.deleteBlocksCommand) == 0)
               .collect(Collectors.toSet());
   ```



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DeadNodeHandler.java:
##########
@@ -87,6 +87,9 @@ public void onMessage(final DatanodeDetails datanodeDetails,
       if (!nodeManager.getNodeStatus(datanodeDetails).isInMaintenance()) {
         removeContainerReplicas(datanodeDetails);
       }
+      
+      // remove commands in command queue for the DN
+      nodeManager.getCommandQueue(datanodeDetails.getUuid());

Review Comment:
   Add a log message for dropping the commands.
   ```
         final List<SCMCommand> commands = nodeManager.getCommandQueue(datanodeDetails.getUuid());
         LOG.info(...);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org