[
https://issues.apache.org/jira/browse/YARN-10101?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17029906#comment-17029906
]
Szilard Nemeth commented on YARN-10101:
---------------------------------------
Hi [~adam.antal],
Good job!
As a general suggestion, next time you could have isolated the refactor part
(LogWebServiceUtils and WrappedLogMetaRequest) into a separate cleanup /
refactor jira then build the core change on top of that.
General comment: I like you created LogServlet#validateUserInput as a
self-contained validator method.
*Some comments (not in priority order):*
1. Nit: In ContainerLogsRequest: Field name, getter/setter naming is not in
sync. I would either rename the field to "appAttemptId" or
"applicationAttemptId", then name the getter/setter accordingly.
2. Nit: In org.apache.hadoop.yarn.client.cli.LogsCLI#runCommand: You are
passing a null applicationAttemptId every time. Is this intentional?
Do you want to file a follow-up jira that adds a CLI switch to query logs by
applicationAttemptId?
ContainerLogsRequest request = new ContainerLogsRequest(appId, null,
Apps.isApplicationFinalState(appState), appOwner, nodeAddress,
null, containerIdStr, localDir, logs, bytes, null);
3. In
org.apache.hadoop.yarn.logaggregation.filecontroller.tfile.LogAggregationTFileController#readAggregatedLogsMeta:
Regarding the added belongsToAppAttempt call: This call is guarded by this if
statement:
{code:java}
if (getAllContainers || (key.toString().equals(containerIdStr))
{code}
Since the if can become true if container ids are matching, what will happen in
this case if belongsToAppAttempt is called with a null appAttemptId? Could this
cause a problem?
4. Please add some javadoc to
org.apache.hadoop.yarn.server.webapp.WrappedLogMetaRequest#getContainerLogMetas.
This seems to be the key method of this class. Visibility could be restricted
to package-private, too.
5. In LogServlet: You added some exception logging as:
{code:java}
LOG.info("Could not obtain node HTTP address from provider.", ex);
{code}
Have you considered changing them to LOG.error? What do you think of this?
6. I can see the following code block has been removed from
org.apache.hadoop.yarn.server.webapp.LogServlet#getContainerLogsInfo:
{code:java}
ContainerId containerId = null;
try {
containerId = ContainerId.fromString(containerIdStr);
} catch (IllegalArgumentException e) {
throw new BadRequestException("invalid container id, " + containerIdStr);
}
{code}
I understand that the container id validation has been moved elsewhere, but I
can't see throw new BadRequestException anywhere, anymore? This seems to be a
problem for me.
7. Nit: org.apache.hadoop.yarn.server.webapp.LogServlet#validateUserInput:
Comment:
{code:java}
// At least field should be set
{code}
Seems to miss an "a" / "one"
8.
org.apache.hadoop.yarn.logaggregation.TestContainerLogsUtils#createContainerLogFileInRemoteFS:
The javadoc for this method has 2 issues
1. containerId has a TODO and this parameter name is not specified anymore
2. deletePreviousRemoteLogDir: this parameter name is not specified anymore
9. In
org.apache.hadoop.mapreduce.v2.hs.webapp.TestHsWebServicesLogs.WebServletModule#configureServlets:
Please add some comments above the mocks: To me it's not clear why the attemps
APP_ATTEMPT_1_2 / APP_ATTEMPT_2_2 and containers CONTAINER_2_2_2 /
CONTAINER_2_2_3 needs special care. I guess it should be in-line with the
javadoc of the class, but it's worth 2 more comments in the code I think.
10. Nit: You could extract
{code:java}
r.path("ws").path("v1")
.path("history").path("containerlogs")
{code}
throughout TestHsWebServicesLogs
11. Nit: In TestHsWebServicesLogs: You could extract the code that creates the
containerID strings from a response.
{code:java}
List<ContainerLogsInfo> responseList =
response.getEntity(new GenericType<List<ContainerLogsInfo>>(){});
Set<String> actualStrings =
responseList.stream()
.map(ContainerLogsInfo::getContainerId)
.collect(Collectors.toSet());
{code}
12. Nit: In TestHsWebServicesLogs: The following code block is repeated:
{code:java}
assertThat(logsInfo.getContainerLogsInfo()).isNotNull();
assertThat(logsInfo.getContainerLogsInfo().size()).isEqualTo(1);
ContainerLogFileInfo fileInfo = logsInfo.getContainerLogsInfo().get(0);
{code}
Can you create a helper method to check if containerLogsInfo is not null, has a
certain size, then return the 0th element of the list?
13. Nit: In TestHsWebServicesLogs: The following code block is repeated:
{code:java}
assertThat(fileInfo.getFileName()).isEqualTo(FILE_NAME);
assertThat(fileInfo.getFileSize()).isEqualTo(
String.valueOf(("Hello-" + cId).length()));
{code}
Can you extract it to a helper method?
14. Nit: In TestHsWebServicesLogs: The following code is repeated:
{code:java}
NM_ID_2.toString().replace(":", "_")
{code}
Can you extract it to a helper method?
15. Nit: In TestHsWebServicesLogs: The following code comment is repeated:
{code:java}
// this informative can be only obtained by the NM.
{code}
I guess you wanted to write "information"
> 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,
> YARN-10101.006.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: [email protected]
For additional commands, e-mail: [email protected]
