[
https://issues.apache.org/jira/browse/HBASE-21071?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16584741#comment-16584741
]
Mingliang Liu edited comment on HBASE-21071 at 8/19/18 6:53 AM:
----------------------------------------------------------------
{quote}
It seems the Builder can be a class within MiniClusterOptions.
{quote}
Thanks [[email protected]]. Yes that's a good suggestion.
{quote}
Should it be StartMiniClusterOptions to tie the new Builder tighter to the
startMiniCluster method? (Will other methods in MiniCluster want to take
options?)
{quote}
Yes this should be {{StartMiniClusterOptions}}. I don't find other methods that
will be using this option class.
{quote}
Does this mean the options should be MiniHBaseClusterOptions and we should
rename this start method to be startMiniHBaseCluster. Would a
MiniHBaseClusterBuilder make sense returning a MiniHBaseCluster instance on
which you called start.
{quote}
I checked this and tried in code for something that only focuses on the
{{MiniHBaseCluster}}. Also followed the idea of creating a
{{MiniHBaseClusterBuilder}} class to build a {{MiniHBaseCluster}} directly.
However, I got two obstacles after a short code walk:
# Option combinations supported by {{startMiniCluster}} also include multiple
HDFS options. To simplify those polymorphic helper methods, it seems easier to
consolidate the hbase/hdfs/zk options in one place {{StartMiniClusterOptions}}.
Specially {{startMiniHBaseCluster}} also accepts this option.
# If we replace {{startMiniCluster()}}, the current build-and-start combo
methods, with creating {{MiniHBaseCluster}} from builder first and calling
start(), all call sites (hundreds) will have to update. Meanwhile,
{{MiniHBaseCluster}} constructor currently initializes the cluster. We will
have to split it to builder phase and start() phase. Some of the methods are
using non-static {{HBaseTestingUtility}} methods to prepare directories. As
[~stack] expects, it will be a very large refactoring patch.
*TL;DR* I take the trade off of being perfect and being affordable change. To
start a {{MiniCluster}} or {{MiniHBaseCluster}} cluster, we first build an
immutable option and then pass it to {{startMiniCluster(option)}} or
{{startMiniHBaseCluster(option)}} methods. If using default option values, we
can avoid build an option and use the other two methods {{startMiniCluster()}}
or {{startMiniHBaseCluster()}}. These four methods sever all use cases we are
targeting in a (hopefully) clear, simple and flexible way.
The v1 patch almost implements this idea, with one exception:
{{startMiniCluster(int numSlaves)}} and {{startMiniCluster(int numMasters, int
numSlaves)}} are not yet removed and replaced with simple builder calls. The
reason is that, there are hundreds of calls of those two methods, and changing
them manually can be error-prone. I'd like to post the v1 patch first for high
level review and Jenkins. If it looks good overall, in the next patch I can
update all other places. I'm fine if we keep one of them as another shortcut by
the way.
Thanks,
was (Author: liuml07):
{quote}
It seems the Builder can be a class within MiniClusterOptions.
{quote}
Thanks [[email protected]]. Yes that's a good suggestion.
{quote}
Should it be StartMiniClusterOptions to tie the new Builder tighter to the
startMiniCluster method? (Will other methods in MiniCluster want to take
options?)
{quote}
Yes this should be {{StartMiniClusterOptions}}. I don't find other methods that
will be using this option class.
{quote}
Does this mean the options should be MiniHBaseClusterOptions and we should
rename this start method to be startMiniHBaseCluster. Would a
MiniHBaseClusterBuilder make sense returning a MiniHBaseCluster instance on
which you called start.
{quote}
I checked this and tried in code for something that only focuses on the
{{MiniHBaseCluster}}. Also followed the idea of creating a
{{MiniHBaseClusterBuilder}} class to build a {{MiniHBaseCluster}} directly.
However, I got two obstacles after a short code walk:
# Option combinations supported by {{startMiniCluster}} also include multiple
HDFS options. To simplify those polymorphic helper methods, it seems easier to
consolidate the hbase/hdfs/zk options in one place {{StartMiniClusterOptions}}.
Specially {{startMiniHBaseCluster}} also accepts this option.
# If we replace {{startMiniCluster()}, the current build-and-start combo
methods, with creating {{MiniHBaseCluster}} from builder first and calling
start(), all call sites (hundreds) will have to update. Meanwhile,
{{MiniHBaseCluster}} constructor currently initializes the cluster. We will
have to split it to builder phase and start() phase. Some of the methods are
using non-static {{HBaseTestingUtility}} methods to prepare directories. As
[~stack] expects, it will be a very large refactoring patch.
*TL;DR* I take the trade off of being perfect and being affordable change. To
start a {{MiniCluster}} or {{MiniHBaseCluster}} cluster, we first build an
immutable option and then pass it to {{startMiniCluster(option)}} or
{{startMiniHBaseCluster(option)}} methods. If using default option values, we
can avoid build an option and use the other two methods {{startMiniCluster()}}
or {{startMiniHBaseCluster()}}. These four methods sever all use cases we are
targeting in a (hopefully) clear, simple and flexible way.
The v1 patch almost implements this idea, with one exception:
{{startMiniCluster(int numSlaves)}} and {{startMiniCluster(int numMasters, int
numSlaves)}} are not yet removed and replaced with simple builder calls. The
reason is that, there are hundreds of calls of those two methods, and changing
them manually can be error-prone. I'd like to post the v1 patch first for high
level review and Jenkins. If it looks good overall, in the next patch I can
update all other places. I'm fine if we keep one of them as another shortcut by
the way.
Thanks,
> HBaseTestingUtility::startMiniCluster() to use builder pattern
> --------------------------------------------------------------
>
> Key: HBASE-21071
> URL: https://issues.apache.org/jira/browse/HBASE-21071
> Project: HBase
> Issue Type: Bug
> Components: test
> Affects Versions: 3.0.0
> Reporter: Mingliang Liu
> Assignee: Mingliang Liu
> Priority: Major
> Attachments: HBASE-21071.000.patch, HBASE-21071.001.patch
>
>
> Currently there are 13 {{startMiniCluster()}} methods to set up a mini
> cluster. I'm not surprised if we have a few more in future. It's good to
> support different combination of optional parameters. We have to pick up one
> of them carefully while still wondering the default values of other
> parameters; if we add a new option, we may bring more new methods.
> One solution is to use builder pattern: create a class {{MiniClusterOptions}}
> along with a static class {{MiniClusterOptionsBuilder}}, create a new method
> {{startMiniCluster(MiniClusterOptions)}}. In {{master}} we delete the old 13
> methods while in branch-2, we deprecate the old 13 methods.
> Thoughts?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
