hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao Chen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-15056) Fix TestUnbuffer#testUnbufferException failure
Date Tue, 05 Dec 2017 19:12:00 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-15056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16279062#comment-16279062
] 

Xiao Chen commented on HADOOP-15056:
------------------------------------

Thanks [~jackbearden] for reporting and fixing this, and [~jzhuge] for the reviews. It's great
to see your first jira is fixing trunk unit test failure, which is very much appreciated by
the community. :)

My understanding is the {{UnsupportedOperationException}} would be a last measure, since any
correctly coded streams would be handled smoothly by HADOOP-15012. On the other hand, since
{{unbuffer}} and {{hasCapability}} are 2 separate methods on 2 classes, a wrongly-coded stream
should not go unnoticed.

Code comments:
- Can we modify the test to something like this?
{code}
  public void testUnbufferException() {
    class BuggyStream extends FSInputStream implements StreamCapabilities {
      // Override what ever is necessary.
    }
    BuggyStream bs = Mockito.mock(BuggyStream.class);
    Mockito.when(bs.hasCapability(Mockito.anyString())).thenReturn(true);

    FSDataInputStream fs = new FSDataInputStream(bs);
    fs.unbuffer();
  }
{code}

- About the message, while it good to have this information when a stream doesn't support
unbuffer, {{WARN}} may be intrusive for some cases. For example, I think it's not unreasonable
for an application to write some general codes to unbuffer regardless, but after this they'll
see WARN. It's true this is the only log for that logger, but they still will catch this and
have to change the log level. {{DEBUG}} may be a better option, where people can opt-in.

- Can we change the text of the {{UnsupportedOperationException}}? Maybe something like {{claims
to have unbuffer capabilty but does not to implement CanUnbuffer}} ?

> Fix TestUnbuffer#testUnbufferException failure
> ----------------------------------------------
>
>                 Key: HADOOP-15056
>                 URL: https://issues.apache.org/jira/browse/HADOOP-15056
>             Project: Hadoop Common
>          Issue Type: Improvement
>    Affects Versions: 2.9.0
>            Reporter: Jack Bearden
>            Assignee: Jack Bearden
>            Priority: Minor
>         Attachments: HADOOP-15056.001.patch, HADOOP-15056.002.patch, HADOOP-15056.003.patch,
HADOOP-15056.004.patch
>
>
> Hello! I am a new contributor and actually contributing to open source for the very first
time. :) 
> I pulled down Hadoop today and when running the tests I encountered a failure with the
TestUnbuffer#testUnbufferException test.
> The unbuffer code has recently gone through some changes and I believe this test case
may have been overlooked. Using today's git commit (659e85e304d070f9908a96cf6a0e1cbafde6a434),
and upon running the test case, there is an expect mock for an exception UnsupportedOperationException
that is no longer being thrown. 
> It would appear that a test like this would be valuable so my initial proposed patch
did not remove it. Instead, I removed the conditions that were guarding the cast from being
able to fire -- as was the previous behavior. Now when we encounter an object that doesn't
have the UNBUFFERED StreamCapability, it will throw an error as it did prior to the recent
changes. 
> Please review and let me know what you think! :D



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message