[
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
