[
https://issues.apache.org/jira/browse/YARN-10031?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17208611#comment-17208611
]
Adam Antal commented on YARN-10031:
-----------------------------------
Thanks for the patch [~gandras]. Sorry for the late with the review - here's my
first round, I will probably need another round (sorry).
- {{ExtendedLogMetaRequest}} should have a {{Builder}}, no setters, and final
fields
- the nomenclature "compare" is unfortunately occupied in Java ({{Comparable}}
interface), and I strongly suggest to choose another name for these. For
example {{ExtendedLogMetaRequest.MatchExpression#compare()}} can be named
{{match()}}
- {{LogAggregationMetaCollector}} 's conf and request object can be final
- {{LogAggregationMetaCollector.collect#L79}}: what if node id is not
provided? ({{logsRequest.getNodeId()}} is null) I think we can get an NPE
there. Also if we allow null values, it worth after catching the exception in
L108 to just continue in the loop.
- I think this is a bit ineffective:
{code:java}
metaFiles.keySet().stream().filter(containerId ->
!(logsRequest.getContainerId() == null ||
logsRequest.getContainerId().equals(containerId)))
.collect(Collectors.toList()).forEach(metaFiles::remove);
{code}
Here you filter a keyset, collect to a list and call a remove for these items
on the original map - I think there's too much intermediate data structure is
created. Can we do a simple loop instead?
- In {{LogAggregationUtils.getNodeString#L301}} you can just return the
{{combineIterators}} directly.
- Please fix the ASF license warnings
- {{LogAggregationIndexedFileController.parseChecksum()}} seems a bit odd to
me:
** it should be private, since it is only called inside the class
** I think it will always return null, because the first condition is always
true (if it is false, then {{getLogMetaFilesOfNode()}} returns with null
previously).
** I suggest to revise the logic in the function.
> Create a general purpose log request with additional query parameters
> ---------------------------------------------------------------------
>
> Key: YARN-10031
> URL: https://issues.apache.org/jira/browse/YARN-10031
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: yarn
> Reporter: Adam Antal
> Assignee: Andras Gyori
> Priority: Major
> Attachments: YARN-10031-WIP.001.patch, YARN-10031.001.patch,
> YARN-10031.002.patch
>
>
> The current endpoints are robust but not very flexible with regards to
> filtering options. I suggest to add an endpoint which provides filtering
> options.
> E.g.:
> In ATS we have multiple endpoints:
> /containers/{containerid}/logs/{filename}
> /containerlogs/{containerid}/{filename}
> We could add @QueryParams parameters to the REST endpoints like this:
> /containers/{containerid}/logs?fileName=stderr&containerState=FAILED&nodeId=nm45
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
