sumitagrawl commented on code in PR #7339:
URL: https://github.com/apache/ozone/pull/7339#discussion_r1818636762
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -344,45 +344,47 @@ private void saveIterationStatistic(Integer
iterationNumber, IterationResult iR)
public List<ContainerBalancerTaskIterationStatusInfo>
getCurrentIterationsStatistic() {
- int lastIterationNumber = iterationsStatistic.stream()
- .mapToInt(ContainerBalancerTaskIterationStatusInfo::getIterationNumber)
- .max()
- .orElse(0);
-
- ContainerBalancerTaskIterationStatusInfo currentIterationStatistic = new
ContainerBalancerTaskIterationStatusInfo(
- lastIterationNumber + 1,
- null,
- getSizeScheduledForMoveInLatestIteration() / OzoneConsts.GB,
- sizeActuallyMovedInLatestIteration / OzoneConsts.GB,
- metrics.getNumContainerMovesScheduledInLatestIteration(),
- metrics.getNumContainerMovesCompletedInLatestIteration(),
- metrics.getNumContainerMovesFailedInLatestIteration(),
- metrics.getNumContainerMovesTimeoutInLatestIteration(),
- findTargetStrategy.getSizeEnteringNodes()
- .entrySet()
- .stream()
- .filter(Objects::nonNull)
- .filter(datanodeDetailsLongEntry ->
datanodeDetailsLongEntry.getValue() > 0)
- .collect(Collectors.toMap(
- entry -> entry.getKey().getUuid(),
- entry -> entry.getValue() / OzoneConsts.GB
- )
- ),
- findSourceStrategy.getSizeLeavingNodes()
- .entrySet()
- .stream()
- .filter(Objects::nonNull)
- .filter(datanodeDetailsLongEntry ->
datanodeDetailsLongEntry.getValue() > 0)
- .collect(
- Collectors.toMap(
- entry -> entry.getKey().getUuid(),
- entry -> entry.getValue() / OzoneConsts.GB
- )
- )
- );
- List<ContainerBalancerTaskIterationStatusInfo> resultList = new
ArrayList<>(iterationsStatistic);
- resultList.add(currentIterationStatistic);
- return resultList;
+ synchronized (iterationsStatistic) {
Review Comment:
What we are going to protect is not clear.
- Do need block multiple client calling getCurrentInterationStatistics() ?
Above change will do this only which do not have any issue, as all are
independent and do not have any context being shared.
I think issue is, when currentStatistics is generated in this method,
parallel operation by container balancer can have impact to this. eg:
1. currentStatistic interation can be duplicated if parallel added by
saveIteration method
2. Some fields iteration may not be thread safe when constructing the
currentStatistic
Please recheck.
--
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]
