[
https://issues.apache.org/jira/browse/YARN-4920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15263284#comment-15263284
]
Junping Du commented on YARN-4920:
----------------------------------
Quick go through current patch, some comments below:
AHSWebService.java,
{code}
+ try {
+ containerId = ContainerId.fromString(containerIdStr);
+ } catch (IllegalArgumentException ex) {
+ return createBadResponse(Status.BAD_REQUEST,
+ "Invalid ContainerId: " + containerIdStr);
+ }
{code}
Status.BAD_REQUEST/HTTP 400 is usually for request URL is malformed. In this
case, it is better to use Status.NOT_FOUND/HTTP 404 to indicate the resource
(containerId) is not found in server side. Please refer RFC2616 for more
details:https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
{code}
+ return createBadResponse(Status.BAD_REQUEST,
+ "The application is not at Running or Finished State.");
{code}
The same case here. We should use NOT_FOUND instead.
{noformat}
+ private boolean parseBooleanParam(String param) {
+ if (param != null) {
+ return ("true").equalsIgnoreCase(param);
+ }
+ return false;
+ }
{noformat}
We can call {{return ("true").equalsIgnoreCase(param);}} directly which should
cover param is null case.
{noformat}
+ private StreamingOutput getStreamingOutput(ApplicationId appId,
+ String appOwner, final String nodeId, final String containerIdStr,
+ final String logFile) throws IOException{
+ String suffix = LogAggregationUtils.getRemoteNodeLogDirSuffix(conf);
+ org.apache.hadoop.fs.Path remoteRootLogDir = new org.apache.hadoop.fs.Path(
{noformat}
Format (indent) issue here.
{noformat}
if (appOwner == null) {
org.apache.hadoop.fs.Path toMatch = LogAggregationUtils
.getRemoteAppLogDir(remoteRootLogDir, appId, "*", suffix);
FileStatus[] matching = fc.util().globStatus(toMatch);
if (matching == null || matching.length != 1) {
return null;
}
remoteAppDir = matching[0].getPath();
}
{noformat}
It is possible for an app to match with the other one, like: abcd_011 can also
be matched as abcd_01. Isn't it? If so, we should fix this issue.
{noformat}
while (true) {
try {
byte[] buf = new byte[65535];
{noformat}
We shouldn't allocate memory of buffer inside of while loop which will cause a
lot of unncessary GC operations in case log file is pretty large. Instead, we
should reuse the limited size buffer.
In NMWebServices.java,
{noformat}
+ if (downloadFile) {
+ response.header("Content-Type", "application/octet-stream");
+ response.header("Content-Disposition", "attachment; filename="
+ + logFile.getName());
+ }
{noformat}
I would suggest to add file's modification time to
response.header("Last-Modified", file_modification_time_truncate_to_seconds)
also. User may need to figure out which app's log could be old and which apps'
log are new which shouldn't depend on file's download time via http. However,
this is just an optional comments, see if you want to address now or later.
> ATS/NM should support a link to dowload/get the logs in text format
> -------------------------------------------------------------------
>
> Key: YARN-4920
> URL: https://issues.apache.org/jira/browse/YARN-4920
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: yarn
> Reporter: Xuan Gong
> Assignee: Xuan Gong
> Attachments: YARN-4920.20160424.branch-2.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
