[
https://issues.apache.org/jira/browse/YARN-10745?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17339816#comment-17339816
]
Eric Badger commented on YARN-10745:
------------------------------------
Hi [~dmmkr], thanks for the patch. Overall I think it has changes that make
sense, but I have a few comments/questions
{noformat}
- if (LOG.isDebugEnabled()) {
- LOG.debug("Auth is SASL user=\"{}\" JAAS context=\"{}\"",
- jaasClientIdentity, jaasClientEntry);
- }
+ LOG.debug("Auth is SASL user=\"{}\" JAAS context=\"{}\"",
+ jaasClientIdentity, jaasClientEntry);
{noformat}
Looks like the wrong indentation here
{noformat}
switch (purgePolicy) {
case SkipOnChildren:
// don't do the deletion... continue to next record
- if (LOG.isDebugEnabled()) {
- LOG.debug("Skipping deletion");
- }
+ LOG.debug("Skipping deletion");
toDelete = false;
break;
case PurgeAll:
// mark for deletion
- if (LOG.isDebugEnabled()) {
- LOG.debug("Scheduling for deletion with children");
- }
+ LOG.debug("Scheduling for deletion with children");
toDelete = true;
entries = new ArrayList<RegistryPathStatus>(0);
break;
case FailOnChildren:
- if (LOG.isDebugEnabled()) {
- LOG.debug("Failing deletion operation");
- }
+ LOG.debug("Failing deletion operation");
throw new PathIsNotEmptyDirectoryException(path);
{noformat}
Same here with the case statements
{noformat}
List<NodeReport> clusterNodeReports = yarnClient.getNodeReports(
NodeState.RUNNING);
- LOG.info("Got Cluster node info from ASM");
+ if (clusterNodeReports.isEmpty()) {
+ LOG.info("Got Empty Cluster node Report info from ASM");
+ }
{noformat}
Is {{clusterNodeReports}} guaranteeed to be non-null here? Otherwise we can NPE
{noformat}
- // NodeManager is the last service to start, so NodeId is available.
+ // NodeStatusUpdater is the last service to start, so NodeId is available.
{noformat}
I'm not sure what this change is for. The comment seems to imply that
NodeStatusUpdater is the last service to start, so the service that populates
NodeId will already be done. Nodemanager is probably the last service to start
overall since it adds all of the other services, but I don't think the change
in the comment makes the code any clearer
{noformat}
+ LOG.info("Callback succeeded for initializing request processing " +
+ "pipeline for an AM ");
{noformat}
Can you comment on the this log statement? Have you found this useful for
debugging? Does it only get logged rarely?
{noformat}
- LOG.info("hostsReader include:{" +
- StringUtils.join(",", hostsReader.getHosts()) +
- "} exclude:{" +
- StringUtils.join(",", hostsReader.getExcludedHosts()) + "}");
-
+ if (!hostsReader.getHosts().isEmpty() ||
+ !hostsReader.getExcludedHosts().isEmpty()) {
+ LOG.info("hostsReader include:{" +
+ StringUtils.join(",", hostsReader.getHosts()) +
+ "} exclude:{" +
+ StringUtils.join(",", hostsReader.getExcludedHosts()) + "}");
+ }
{noformat}
I feel like we're losing information here. Knowing that the hostsReader is
empty is helpful. We could log it differently, but I don't think we want to
lose that information
> Change Log level from info to debug for few logs and remove unnecessary
> debuglog checks
> ---------------------------------------------------------------------------------------
>
> Key: YARN-10745
> URL: https://issues.apache.org/jira/browse/YARN-10745
> Project: Hadoop YARN
> Issue Type: Improvement
> Reporter: D M Murali Krishna Reddy
> Assignee: D M Murali Krishna Reddy
> Priority: Minor
> Attachments: YARN-10745.001.patch, YARN-10745.002.patch,
> YARN-10745.003.patch, YARN-10745.004.patch
>
>
> Change the info log level to debug for few logs so that the load on the
> logger decreases in large cluster and improves the performance.
> Remove the unnecessary isDebugEnabled() checks for printing strings without
> any string concatenation
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
