[
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
