adoroszlai commented on code in PR #4453:
URL: https://github.com/apache/ozone/pull/4453#discussion_r1145798719


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -627,21 +614,20 @@ public void sendDatanodeCommand(SCMCommand<?> command,
    * @param target The datanode which will receive the command.
    * @param scmDeadlineEpochMs The epoch time in ms, after which the command
    *                           will be discarded from the SCMPendingOps table.
-   * @param datanodeDeadlineEpochMs The epoch time in ms, after which the
-   *                                command will be discarded on the datanode 
if
-   *                                it has not been processed.
    * @throws NotLeaderException
    */
   public void sendDatanodeCommand(SCMCommand<?> command,
       ContainerInfo containerInfo, DatanodeDetails target,
-      long scmDeadlineEpochMs, long datanodeDeadlineEpochMs)
+      long scmDeadlineEpochMs)
       throws NotLeaderException {
+    long datanodeDeadline =
+        scmDeadlineEpochMs - rmConf.getDatanodeTimeoutOffset();

Review Comment:
   I don't think negative deadline could be a problem.  Offset would have to be 
53+ years (and counting) for deadline to be negative.
   
   However, validation is a good idea.  It can be done in 
`ReplicationManagerConfig#validate`, since `scmDeadlineEpochMs` is also 
calculated based on a config property.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -1169,15 +1154,6 @@ public void setMaintenanceReplicaMinimum(int 
replicaCount) {
     )
     private boolean push = true;
 
-    @PostConstruct
-    public void validate() {
-      if (!(commandDeadlineFactor > 0) || (commandDeadlineFactor > 1)) {
-        throw new IllegalArgumentException("command.deadline.factor is set to "
-            + commandDeadlineFactor
-            + " and must be greater than 0 and less than equal to 1");
-      }
-    }

Review Comment:
   Let's keep `validate()` and check that `datanodeTimeoutOffset < 
eventTimeout` (maybe even enforce some minimum difference).



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to