[GitHub] [ozone] aswinshakil commented on a diff in pull request #3867: HDDS-7370. Add pending commands in SCM to Datanode command count

[GitBox <gi...@apache.org>]

aswinshakil commented on code in PR #3867:
URL: https://github.com/apache/ozone/pull/3867#discussion_r1006135381


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -305,8 +314,19 @@ public void setCommandCounts(CommandQueueReportProto cmds) {
               "Setting it to zero", cmdCount, this);
           cmdCount = 0;
         }
+        cmdCount += mutableCmds.getOrDefault(command, 0);
+        // Each CommandType will be in the report once only. So we remove any
+        // we have seen, so we can add anything the DN has not reported but
+        // there is a command queued for. The DNs should return a count for all
+        // command types even if they have a zero count, so this is really to
+        // handle something being wrong on the DN where it sends a spare report.
+        // It really should never happe.

Review Comment:
   ```suggestion
           // It really should never happen.
   ```



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeInfo.java:
##########
@@ -286,11 +286,20 @@ public void setNodeStatus(NodeStatus newNodeStatus) {
    * Set the current command counts for this datanode, as reported in the last
    * heartbeat.
    * @param cmds Proto message containing a list of command count pairs.
+   * @param commandsToBeSent Summary of commands which will be sent to the DN
+   *                         as the heartbeat is processed and should be added
+   *                         to the command count.
    */
-  public void setCommandCounts(CommandQueueReportProto cmds) {
+  public void setCommandCounts(CommandQueueReportProto cmds,
+      Map<SCMCommandProto.Type, Integer> commandsToBeSent) {
     try {
       int count = cmds.getCommandCount();
+      Map<SCMCommandProto.Type, Integer> mutableCmds
+          = new HashMap<>(commandsToBeSent);
       lock.writeLock().lock();
+      // Purge the existing counts, as each report should completely replace
+      // the existing counts.
+      commandCounts.clear();

Review Comment:
   Just a quick question, why was this not done before and done now? I understand we would like to keep the current count and replace the stale counters. I thought it was already done by `commandCounts.put(command, cmdCount)` and also now with `commandCounts.putAll(mutableCmds)`. 



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