adoroszlai commented on code in PR #7776:
URL: https://github.com/apache/ozone/pull/7776#discussion_r1934216512
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ClosedContainerReplicator.java:
##########
@@ -122,20 +130,13 @@ public Void replicate() throws Exception {
replicationTasks = new ArrayList<>();
for (ContainerInfo container : containerInfos) {
-
- final ContainerWithPipeline containerWithPipeline =
- containerOperationClient
- .getContainerWithPipeline(container.getContainerID());
-
if (container.getState() == LifeCycleState.CLOSED) {
-
- final List<DatanodeDetails> datanodesWithContainer =
- containerWithPipeline.getPipeline().getNodes();
-
+ final Pipeline pipeline =
containerOperationClient.getPipeline(container.getPipelineID().getProtobuf());
+ final List<DatanodeDetails> datanodesWithContainer =
pipeline.getNodes();
final List<String> datanodeUUIDs =
- datanodesWithContainer
- .stream().map(DatanodeDetails::getUuidString)
- .collect(Collectors.toList());
+ datanodesWithContainer
+ .stream().map(DatanodeDetails::getUuidString)
+ .collect(Collectors.toList());
Review Comment:
nit: please restore previous indent
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationSupervisor.java:
##########
@@ -240,6 +240,22 @@ public void addTask(AbstractReplicationTask task) {
return;
}
+ initCounters(task);
+
+ if (inFlight.add(task)) {
+ if (task.getPriority() != ReplicationCommandPriority.LOW) {
+ // Low priority tasks are not included in the replication queue sizes
+ // returned to SCM in the heartbeat, so we only update the count for
+ // priorities other than low.
+ taskCounter.computeIfAbsent(task.getClass(),
+ k -> new AtomicInteger()).incrementAndGet();
+ }
+ queuedCounter.get(task.getMetricName()).incrementAndGet();
+ executor.execute(new TaskRunner(task));
+ }
Review Comment:
Please extract this to a new method, e.g. `addToQueue`, placed right after
`initCounters` in the file. Benefits:
- more uniform abstraction level for steps in `addTask`: `initCounters` is
higher level than this block
- reduce the change triggered by extracting `initCounters`, whose code was
previously above this block
Optionally the check for max. queue size can also be extracted. Then
`addTask` would look like:
```java
if (queueHasRoomFor(task)) {
initCounters(task);
addToQueue(task);
}
```
(Please feel free to tweak these names.)
--
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]
