[ 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: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org