adoroszlai commented on code in PR #5611:
URL: https://github.com/apache/ozone/pull/5611#discussion_r1395235166
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -100,6 +102,9 @@ public class DeleteBlocksCommandHandler implements
CommandHandler {
private final long tryLockTimeoutMs;
private final Map<String, SchemaHandler> schemaHandlers;
+ @VisibleForTesting
+ private long deleteCmdWorkerInterval;
Review Comment:
`@VisibleForTesting` is unnecessary, since the member is `private`, not
visible to others.
Also, no need to store the interval in both classes. Let's keep the one in
`DeleteCmdWorker`.
```suggestion
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -121,7 +126,14 @@ public DeleteBlocksCommandHandler(OzoneContainer container,
dnConf.getBlockDeleteThreads(), threadFactory);
this.deleteCommandQueues =
new LinkedBlockingQueue<>(dnConf.getBlockDeleteQueueLimit());
- handlerThread = new Daemon(new DeleteCmdWorker());
+ long interval = this.conf.getLong(HddsConfigKeys.
+ HDDS_COMMAND_DELETE_WORKER_INTERVAL,
+ HddsConfigKeys.HDDS_COMMAND_DELETE_WORKER_INTERVAL_DEFAULT);
+ Preconditions.checkState(interval > 0,
+ "The value of %s must be greater than 0.",
+ HddsConfigKeys.HDDS_COMMAND_DELETE_WORKER_INTERVAL);
Review Comment:
After moving the property to `DatanodeConfiguration`, this check can be
moved to `validate()` there:
https://github.com/apache/ozone/blob/366c2fb57ae15acbb4c357086328ddec2faeb4f3/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java#L535-L543
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -221,6 +238,21 @@ public boolean isLockAcquisitionFailed() {
*/
public final class DeleteCmdWorker implements Runnable {
+ private long interval;
+
+ public DeleteCmdWorker() {
+ this(HddsConfigKeys.HDDS_COMMAND_DELETE_WORKER_INTERVAL_DEFAULT);
+ }
Review Comment:
I don't think this is needed.
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -1519,6 +1519,14 @@
executed since last report. Unit could be defined with
postfix (ns,ms,s,m,h,d)</description>
</property>
+ <property>
+ <name>hdds.command.delete.worker.interval</name>
+ <value>2000</value>
+ <tag>OZONE, DATANODE, MANAGEMENT</tag>
+ <description>The interval between DeleteCmdWorker execution of delete
+ commands. The unit is milliseconds.
+ </description>
+ </property>
Review Comment:
Please add the new config property to `DatanodeConfiguration` instead of
`ozone-default.xml`.
There you'll find other properties related to block deletion, e.g.:
https://github.com/apache/ozone/blob/366c2fb57ae15acbb4c357086328ddec2faeb4f3/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java#L172-L185
Please use the same naming convention for the new key, e.g.
`block.delete.command.worker.interval`. (Prefix `hdds.datanode.` is
automatically added.)
--
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]
