[
https://issues.apache.org/jira/browse/HDFS-4352?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13547723#comment-13547723
]
Suresh Srinivas commented on HDFS-4352:
---------------------------------------
bq. That is inevitable when it comes to matters of style.
It is not a matter of style. I have explained why the patch in this jira
actually made things worse.
bq. 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.
We seem to be repeating the same thing. Assuming you are talking about
MiniDFSCluster, when you say DFSMiniCluster, the problem solved there is
different. I have explained this earlier, perhaps it is not very clear. Let me
explain:
The builder is added because a single class (MiniDFSCluster) had many
constructors, each constructor taking different combination of parameters. A
single class had whole bunch of constructors, that is, the class had *
telescoping constructor pattern *. Using a builder, a whole bunch of
constructors were replaced. We were also seeing need for adding more and more
constructors to MiniDFSCluster as we added more features. If we did not add new
constructors, we would break a large number of places where MiniDFSCluster is
constructed in tests. All these were fixed by builder.
Here you have a single method in the class. It is the callers that are setting
some fields as they need. Also I do not see the list of attributes changing as
much as MiniDFSCluster. To me adding a builder does not make much difference.
The change here motivated to replace a long list of parameters. I disagree that
it requires builder. This is not an argument about preference/style. I believe
builder is the wrong thing to use here.
As regards pointing out issue with the existing code, you could have fixed
documentation errors that you found. That does not some how make adding builder
justified.
I have not looked at HDFS-4353. But if you were making similar change as this
jira, then reverting following the discussion in this jira is justified. If you
think the discussion in this jira is not relevant to that, please feel free to
revert the revert.
BTW Can you folks create a branch for HDFS-347 and do this change in a branch
instead of trunk?
> 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
