Gera Shegalov commented on YARN-2934:

Thanks for the latest patch. good to see the patch lose 3kb, most of all there 
are no more changes to the common Configuration class.

One checkstyle issue is the 80-column warning is from the patch around:
        long tailSizeInBytes = conf.getLong(

Those are pretty long names:
Can we do: 
and the corresponding default. Having stderr in name is also great for users to 
understand what error file is meant in 99% of the cases.
Same thing is for container.stderr.pattern

Still don't see any value in this, please drop:
        if (listStatus.length > 1) {
          LOG.error("Multiple files in " + containerLogDir
              + ", seems to match the error file name pattern configured. "
              + Arrays.toString(listStatus));

Let us not guard the tail read:
        if (fileSize != 0) {
and there is a value in seeing that the file is empty already on the 

Instead of 

call cleanup so we can pass the logger
IOUtils.cleanup(LOG, errorFileIS)

Since the trunk is on JDK7 min:
We can drop the constant UTF_8 and use
new String(tailBytes, StandardCharsets.UTF_8)

listStatus as a name for a variable is not intuitive. Maybe use errFileStatus 
for that. 

Obviously I meant tailSizeInBytes, thanks for paying attention. Agree that RLFS 
file status toString might look too ugly.

We can FileUtil.stat2Paths or add a loop here to extract just the last path 

Also realizing that we should have a low cap on the tail size to prevent a 
misconfiguration to knock out NM with OOM on container failures since we do:
byte[] tailBytes = new byte[bufferSize];

One can easily see why I initially confused tailBytes for an int. It should be 
called along the lines {code}tailBuffer{code} 

> Improve handling of container's stderr 
> ---------------------------------------
>                 Key: YARN-2934
>                 URL: https://issues.apache.org/jira/browse/YARN-2934
>             Project: Hadoop YARN
>          Issue Type: Improvement
>            Reporter: Gera Shegalov
>            Assignee: Naganarasimha G R
>            Priority: Critical
>         Attachments: YARN-2934.v1.001.patch, YARN-2934.v1.002.patch, 
> YARN-2934.v1.003.patch, YARN-2934.v1.004.patch, YARN-2934.v1.005.patch, 
> YARN-2934.v1.006.patch, YARN-2934.v1.007.patch, YARN-2934.v1.008.patch, 
> YARN-2934.v2.001.patch
> Most YARN applications redirect stderr to some file. That's why when 
> container launch fails with {{ExitCodeException}} the message is empty.

This message was sent by Atlassian JIRA

Reply via email to