slfan1989 commented on code in PR #8705:
URL: https://github.com/apache/hadoop/pull/8705#discussion_r3888732102
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/AppLogAggregatorImpl.java:
##########
@@ -332,7 +332,7 @@ private void uploadLogsForContainers(boolean appFinished)
LOG.debug("Cycle #{} of log aggregator", logAggregationTimes);
String diagnosticMessage = "";
boolean logAggregationSucceedInThisCycle = true;
- DeletionTask deletionTask = null;
+ List<DeletionTask> deletionTasks = new ArrayList<>();
Review Comment:
Could we add a regression test for the actual multi-container case?
The existing `TestAppLogAggregatorImpl` coverage exercises only one
container, so both the original single-variable implementation and this patch
pass the current suite.
The test class already provides `AppLogAggregatorInTest` and a mocked
`DeletionService` that inspects `FileDeletionTask#getBaseDirs()`.
Could we extend it to start two containers with distinct log files and
assert that the deletion tasks contain files from both containers?
This is the exact cardinality that triggers `YARN-11963` and would prevent
the overwrite bug from being reintroduced.
--
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]
