brumi1024 commented on code in PR #5784:
URL: https://github.com/apache/hadoop/pull/5784#discussion_r1267794737
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/RMAppImpl.java:
##########
@@ -1088,8 +1088,15 @@ public void transition(RMAppImpl app, RMAppEvent event) {
// otherwise, add it to ranNodes for further process
app.ranNodes.add(nodeAddedEvent.getNodeId());
- app.logAggregation.addReportIfNecessary(
- nodeAddedEvent.getNodeId(), app.getApplicationId());
+ if (!nodeAddedEvent.isCreatedFromAcquiredState()) {
+ app.logAggregation.addReportIfNecessary(
+ nodeAddedEvent.getNodeId(), app.getApplicationId());
+ } else {
+ LOG.warn(String.format("Not considering container for log aggregation "
+ + "while app is transitioning from ACQUIRED directly to
RELEASED "
+ + "for nodeId: %s and appId: %s",
+ nodeAddedEvent.getNodeId(), app.getApplicationId()));
+ }
Review Comment:
I still have one nit about this log message: the app is not transitioning
from ACQUIRED to RELEASED, one specific container is.
However I'm not sure if we should even log this, it seems to be a bit out of
place here, talking about container transitions in an app transition. Do we
need to know about a container that never actually did anything and that it's
skipped from log aggregation, especially in a warning level?
--
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]
