[
https://issues.apache.org/jira/browse/YARN-6877?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16148098#comment-16148098
]
Junping Du commented on YARN-6877:
----------------------------------
Thanks Xuan for uploading a patch!
Looks like in latest patch, issue still exists for my previous comments:
{noformat}
public void testFetchApplictionLogsHar() throws Exception {
String remoteLogRootDir = "target/logs/";
- Configuration configuration = new Configuration();
+ Configuration configuration = new YarnConfiguration();
{noformat}
Still lacking space between } and catch in some places:
{noformat}
}catch (NumberFormatException ne) {
{noformat}
Some new comments:
In LogAggregationWebUtils.java, verifyAndGetLogStartIndex() is better to change
to getLogStartIndex(). Normally we should check valid of input if we start with
verify*, like original methods - verifyAndGetLogLimits(), but here we just
simply throw exceptions (if input is illegal) and rely on caller to catch
exception and handle. I think call getLogStartIndex() should be better. Same
comments for verifyAndGetLogEndIndex(). But other verfiy* methods should be
fine - at least no exception for caller to catch and handle.
In LogCLIHelpers.dumpAContainerLogsForLogType():
{noformat}
+ return 0;
+ } catch (IOException ex) {
+ System.err.println(ex.getMessage());
return -1;
}
{noformat}
It looks like we swallow IOException here instead of throw it out as previous
behavior. Does this expected? If not, better to keep the original way.
The same comments for dumpAllContainersLogs().
I miss one item in previous review for LogAggregationTFileController.java,
{noformat}
+ @Override
+ public void closeWriter() {
+ this.writer.close();
+ }
{noformat}
Do we need to set writer here to null after we close it which is consistent
with previous behavior? The previous intention seems like to keep minimum
retain memory. The reason is still valid here. Isn't it?
Other looks fine to me.
btw, Findbugs warning shouldn't be related. Can you please check checkstyle,
javadoc warnings here?
> Create an abstract log reader for extendability
> -----------------------------------------------
>
> Key: YARN-6877
> URL: https://issues.apache.org/jira/browse/YARN-6877
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Xuan Gong
> Assignee: Xuan Gong
> Attachments: YARN-6877-branch-2.001.patch, YARN-6877-trunk.001.patch,
> YARN-6877-trunk.002.patch, YARN-6877-trunk.003.patch,
> YARN-6877-trunk.004.patch, YARN-6877-trunk.005.patch,
> YARN-6877-trunk.006.patch
>
>
> Currently, TFile log reader is used to read aggregated log in YARN. We need
> to add an abstract layer, and pick up the correct log reader based on the
> configuration.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
