Copilot commented on code in PR #10307:
URL: https://github.com/apache/ozone/pull/10307#discussion_r3270777335
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerLogger.java:
##########
@@ -181,40 +181,38 @@ public static void logReconciled(ContainerData
containerData, long oldDataChecks
/**
* Logged when a container is successfully moved from one data volume to
another.
*
- * @param containerId The ID of the moved container.
+ * @param containerData The container after it has been moved to the
destination volume.
* @param sourceVolume The source volume path.
* @param destinationVolume The destination volume path.
* @param containerSize The size of data moved from container in bytes.
* @param timeTaken The time taken for the move in milliseconds.
*/
- public static void logMoveSuccess(long containerId, StorageVolume
sourceVolume,
+ public static void logMoveSuccess(ContainerData containerData, StorageVolume
sourceVolume,
StorageVolume destinationVolume, long containerSize, long timeTaken) {
- LOG.info(getMessage(containerId, sourceVolume, destinationVolume,
containerSize, timeTaken));
+ LOG.info(getMessage(containerData,
+ "SrcVolume=" + sourceVolume,
+ "DestVolume=" + destinationVolume,
+ "Size=" + containerSize + " bytes",
+ "TimeTaken=" + timeTaken + " ms",
+ "Container is moved from SrcVolume to DestVolume"));
}
Review Comment:
Please add/extend a test to assert the move-success container log line
includes the expected keyworded columns (ID/Index/BCSID/State + move fields).
There are existing DiskBalancer task tests; capturing the
`ContainerLogger.LOG_NAME` logger output during a successful move would help
prevent future regressions in the log format relied on by tooling/parsers.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
@@ -636,8 +637,10 @@ public BackgroundTaskResult call() {
if (moveSucceeded) {
// Add current old container to pendingDeletionContainers.
pendingDeletionContainers.put(System.currentTimeMillis() +
replicaDeletionDelay, container);
- ContainerLogger.logMoveSuccess(containerId, sourceVolume,
- destVolume, containerSize, Time.monotonicNow() - startTime);
+ if (newContainer != null) {
+ ContainerLogger.logMoveSuccess(newContainer.getContainerData(),
sourceVolume,
+ destVolume, containerSize, Time.monotonicNow() - startTime);
+ }
Review Comment:
In the success path, `moveSucceeded` can remain true for unchecked
exceptions (only `IOException` is caught). If an unchecked exception occurs
before `newContainer` is assigned, the `finally` block will still enqueue the
old container for deletion and record success metrics, which risks deleting the
only good replica. Consider catching `Exception` (or at least
`RuntimeException`) to set `moveSucceeded=false`, and/or assert `newContainer`
is non-null when `moveSucceeded` is true and skip pending-deletion bookkeeping
otherwise.
--
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]
