hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-4352) Encapsulate arguments to BlockReaderFactory in a class
Date Wed, 09 Jan 2013 07:02:14 GMT

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

Colin Patrick McCabe commented on HDFS-4352:
--------------------------------------------

Hi Suresh,

I understand that you may not have liked some things about the Params class.  That is inevitable
when it comes to matters of style.

However, earlier today, HDFS-4353 added full JavaDoc for every for every member of {{BlockReaderFactory#Params}}
and asserts to ensure that all essential parameters were set.  I mentioned this several times
but never got a response from you or Nicholas.  I don't think it's fair to keep bringing up
the fact that the original patch had some missing JavaDoc when it has already been addressed.

Actually the revert restored the old code, which has a ton of missing and incorrect JavaDoc.
 For example, it says that {{BlockReaderFactory#newBlockReader}} returns null on error, which
is blatantly false.  It throws an exception on error-- there is no code path that returns
null.  There's no JavaDoc for {{encryptionKey}} or {{ioStreams}}.  There's very little explanation
of a lot of things-- for example, the fact that len can be -1 or what it means if it is. 
It doesn't explain which parameters can be null and which can't, or what happens if they are
null.

bq. Please check my comment from earlier. The builder is useful when we 10 different constructor
with various combinations of parameters. You can consolidate them into a single constructor
by using a builder. I do not think that is the case here. I believe the link you posted above
is along the similar lines.

We currently have two different {{BlockReaderFactory#newBlockReader}} functions and almost
no callers set all of the parameters.  If you think the current code is good style, we could
easily refactor DFSMiniCluster into the same style.  We'd just have a single function with
30 parameters, most of wh ich were optional.

bq. I have hard time understanding the community discussion part. The way I read it, I thought
you were okay with revert to.

Since there were obviously differences of opinion, I was ok with a revert of this JIRA-- HDFS-4352.
 What is hard to explain is why HDFS-4353 was reverted.

HDFS-4353 is a separate JIRA from this which is not even really related.  There's no reason
to revert HDFS-4353 without discussion, which is what happened here.  The changes don't depend
on each other-- I mentioned that before in this thread too.
                
> Encapsulate arguments to BlockReaderFactory in a class
> ------------------------------------------------------
>
>                 Key: HDFS-4352
>                 URL: https://issues.apache.org/jira/browse/HDFS-4352
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: hdfs-client
>    Affects Versions: 2.0.3-alpha
>            Reporter: Colin Patrick McCabe
>         Attachments: 01b.patch, 01.patch
>
>
> Encapsulate the arguments to BlockReaderFactory in a class to avoid having to pass around
10+ arguments to a few different functions.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message