[ https://issues.apache.org/jira/browse/YARN-10101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17027407#comment-17027407 ]
Peter Bacsko edited comment on YARN-10101 at 1/31/20 11:12 AM: --------------------------------------------------------------- [~adam.antal] I quickly went through the patch. Haven't seen anything that stands out, but I suggest some enhancements. 1. There is one part of the code which is a bit hard to read. I'm talking about {{validateUserInput()}}. Lot of nested ifs, I think those can be simplified. For example: {noformat} if (applicationAttemptId != null) { if (!applicationAttemptId.equals(containerId .getApplicationAttemptId())) { ... {noformat} I think this to ifs can be merged to a single condition: {{if (applicationAttemptId != null && (!applicationAttemptId.equals(containerId.getApplicationAttemptId())}} This one, too: {noformat} if (applicationAttemptId != null) { if (applicationId != null) { if (!applicationId.equals(applicationAttemptId.getApplicationId())) { {noformat} --> {{if (applicationAttemptId != null && applicationId != null && !applicationId.equals(applicationAttemptId.getApplicationId())}} Before this condition, I would add a single line comment like "// We have no containerId" to emphasize when that branch is taken. 2. These stuff: {noformat} } catch (Exception ignore) { } {noformat} I'm assuming that you're ignoring the exception because certain input data (appIdStr / appAttemptIdStr / containerIdStr) can be null. I'd rather you checked for null explicitly, then parse it if non-null: {noformat} ApplicationId appId = null; if (appIdStr != null) { try { appId = ApplicationId.fromString(appIdStr); } catch (Exception e) { throw new WebApplicationException("Illegal Application ID string", e); } } {noformat} 3. Do we need {{@InterfaceAudience}} on {{getLogServlet()}} and {{setLogServlet()}}? It's not really part of an interface that is used either internally or externally, as it's stated clearly by {{@VisibleForTesting}}. was (Author: pbacsko): [~adam.antal] I quickly went through the patch. Haven't seen anything that stands out, but I suggest some enhancements. 1. There is one part of the code which is a bit hard to read though. I'm talking about {{validateUserInput()}}. Lot of nested ifs, I think those can be simplified. For example: {noformat} if (applicationAttemptId != null) { if (!applicationAttemptId.equals(containerId .getApplicationAttemptId())) { ... {noformat} I think this to ifs can be merged to a single condition: {{if (applicationAttemptId != null && (!applicationAttemptId.equals(containerId.getApplicationAttemptId())}} This one, too: {noformat} if (applicationAttemptId != null) { if (applicationId != null) { if (!applicationId.equals(applicationAttemptId.getApplicationId())) { {noformat} --> {{if (applicationAttemptId != null && applicationId != null && !applicationId.equals(applicationAttemptId.getApplicationId())}} Before this condition, I would add a single line comment like "// We have no containerId" to emphasize when that branch is taken. 2. These stuff: {noformat} } catch (Exception ignore) { } {noformat} I'm assuming that you're ignoring the exception because certain input data (appIdStr / appAttemptIdStr / containerIdStr) can be null. I'd rather you checked for null explicitly, then parse it if non-null: {noformat} ApplicationId appId = null; if (appIdStr != null) { try { appId = ApplicationId.fromString(appIdStr); } catch (Exception e) { throw new WebApplicationException("Illegal Application ID string", e); } } {noformat} 3. Do we need {{@InterfaceAudience}} on {{getLogServlet()}} and {{setLogServlet()}}? It's not really part of an interface that is used either internally or externally, as it's stated clearly by {{@VisibleForTesting}}. > Support listing of aggregated logs for containers belonging to an application > attempt > ------------------------------------------------------------------------------------- > > Key: YARN-10101 > URL: https://issues.apache.org/jira/browse/YARN-10101 > Project: Hadoop YARN > Issue Type: Sub-task > Components: log-aggregation, yarn > Affects Versions: 3.3.0 > Reporter: Adam Antal > Assignee: Adam Antal > Priority: Major > Attachments: YARN-10101.001.patch, YARN-10101.002.patch, > YARN-10101.003.patch, YARN-10101.004.patch, YARN-10101.005.patch > > > To display logs without access to the timeline server, we need an interface > where we can query the list of containers with aggregated logs belonging to > an application attempt. > We should add support for this. -- 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