hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ravi Phulari (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-6261) Junit tests for FileContextURI
Date Fri, 18 Sep 2009 18:29:16 GMT

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

Ravi Phulari commented on HADOOP-6261:

Cos,thanks for valuable comments and reviewing patch.

bq. indentation: it supposes to be 2 spaces, not 4
Corrected in next patch. 

what is the point of making the parent class abstract. If you want to emphasize the fact that
some of the methods are going to be overridden you can {{@Override} annotation where it's

I'd suggest to change the name of FileContextURIBaseTest to FileContextURIBase because the
first reaction is to suggest to rename the class into Test... so it'd be picked up by a hardness

do you want to make this a @BeforeClass method? Otherwise it kinda stands there along
       for (int i = 0; i < data.length; i++) {
           data[i] = (byte) (i % 10);
I think createFile has been written line 137 times already - I think you don't need an extra
As per our discussion you suggested to disregard above comments .
in qualifiedPath(String pathString, FileContext fc you, perhaps, should avoid using a full
type name as a part of the var. name. You might want to use hungarian notation, e.g. prefix
a String variable with 's', etc. Also, what if fc would be equals to null?

I haven't seen hungarian notation used in most of Hadoop code and there is strong opposition
to use hungarian notation.

bq.it isn't necessary to prefix your test methods with word 'test' since you have the annotation
in place. Although, there's no single rule about this
Changed .

bq. tearDown takes care about cleaning of fs2. Do you need to do the same for fs1?
I guess you meant fc2 and fc1. These 2 are file context pointing to same Dir/Files so cleaning
fc2 in tearDown is enough, no need to clean fc1.

there's a number of places in the code where you have things like
       String fileName = "testFile";
       Path testPath = qualifiedPath(fileName, fc2);
and which is an only one usage of fileName variable. It's better be declared final in this
case or simply use the literal constant in the call directly.
This was intentional for clarity. 

I agree with your way of writing testListStatus, I will update that in patch.

> Junit tests for FileContextURI
> ------------------------------
>                 Key: HADOOP-6261
>                 URL: https://issues.apache.org/jira/browse/HADOOP-6261
>             Project: Hadoop Common
>          Issue Type: Test
>          Components: test
>            Reporter: Ravi Phulari
>            Assignee: Ravi Phulari
>         Attachments: HADOOP-6261.patch
> FileContextURI unit tests. 

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message