Correction from my previous email: At this point, the MiniAccumuloCluster's interface of the MiniAccumuloClusterImpl's interface.
should read At this point, the MiniAccumuloCluster's interface is a subset of the MiniAccumuloClusterImpl's interface. On Wed, Mar 26, 2014 at 1:10 PM, William Slacum < [email protected]> wrote: > [NOTE: I started this email when this thread was new, and it kinda of blew > up on me while writing it and being distracted. Apologies in advance if > things were already covered or it's not relevant any more.] > > Is this a design quality discussion or a a functionality discussion? > > The changes from 1.5->1.6 seem like a poor design decision, but they do > aid in functionality. > > From 1.5: > public MiniAccumuloCluster(File dir, String rootPassword) throws > IOException > public MiniAccumuloCluster(MiniAccumuloConfig config) throws IOException > public void start() throws IOException, InterruptedException > public String getInstanceName() > public String getZooKeepers() > public void stop() throws IOException, InterruptedException > > From 1.6: > public MiniAccumuloCluster(File dir, String rootPassword) throws > IOException > public MiniAccumuloCluster(MiniAccumuloConfig config) throws IOException > public void start() throws IOException, InterruptedException > public Set<Pair<ServerType,Integer>> getDebugPorts() > public String getInstanceName() > public String getZooKeepers() > public void stop() throws IOException, InterruptedException > public MiniAccumuloConfig getConfig() > public Connector getConnector(String user, String passwd) throws > AccumuloException, AccumuloSecurityException > public ClientConfiguration getClientConfig() > > From a client perspective, I see a difference of #getDebugPorts, > #getConfig, #getConnector, #getClientConfig. The other methods are on the > Impl. There's nothing wrong with using aggregation in this case, since the > code would be the same regardless. > > I don't quite understand what it means to "extend generically." At this > point, the MiniAccumuloCluster's interface of the MiniAccumuloClusterImpl's > interface. The naming could, and should, be better, but I don't quite get > where we're losing functionality. > > > > On Wed, Mar 26, 2014 at 12:06 PM, Keith Turner <[email protected]> wrote: > >> There were many change made to MAC so Accumulo could test itself. For >> example a method was added to return the internal threads that flush logs. >> Flushing the logs may be a useful feature to add. However it could be >> offered in a way that does not expose these internal threads. When >> working on ACCUMULO-2151 I had no desire to reimplement things like this, >> I just wanted to hide it. It was hidden from users so we do not have to >> support it and can change it at will when testing 1.7.0. >> >> As Sean said MAC was a class in 1.4.4, 1.5.0, and 1.5.1. So making it an >> interface would break things for any users using it. Any reorganizing of >> the implementation of MAC could easily be done after 1.6.0. From a users >> perspective the MAC API has changed very little, even though the >> implementation has dramatically changed. >> >> >> >> >> On Wed, Mar 26, 2014 at 3:10 AM, Sean Busbey <[email protected] >> >wrote: >> >> > ACCUMULO-2143 has developed a conversation about MiniAccumuloCluster's >> > intended use and the way we currently implement the difference between >> MAC >> > for external use and MAC for internal Accumulo testing[1]. >> > >> > In particular, Josh had a few major concerns >> > >> > ----- >> > >> > It doesn't make sense to me why MiniAccumuloCluster is a concrete class >> > which, ultimately still tied to a MiniAccumuloClusterImpl. >> > MiniAccumuloCluster *requires* a MiniAccumuloClusterImpl or something >> that >> > extends it. This is what's really chafing me about the separation of >> > "accumulo user" and "accumulo developer" methods - you *always* have >> them >> > both. Not to mention, this hierarchy is really obnoxious to create a new >> > implementation of AccumuloMiniCluster(Impl) because I have to carry all >> of >> > the cruft of the "original" implementation with me. >> > >> > Bringing this back around to this ticket, while I still don't agree with >> > the reasoning that exposing the FileSystem or ZooKeeper object that >> > MiniAccumuloClusterImpl is getting us anything other than the ability to >> > say "we didn't change this [arbitrary] API". For "users" who might not >> care >> > what the underlying FileSystem or ZooKeeper connection, it's merely an >> > extra two items in their editor's code-completion. For "users" who would >> > care to use this information, we now make them jump through extra hoops >> to >> > get it. That just doesn't make any sense to me for something we haven't >> > even released. >> > >> > To be honest, I really want to re-open >> > ACCUMULO-2151<https://issues.apache.org/jira/browse/ACCUMULO-2151>, >> > make MiniAccumuloCluster an interface, MiniAccumuloClusterImpl an >> > implementation of said interface, and create some factory class to make >> > instances, ala Connector.tableOperations, Connector.securityOperations, >> > etc. Right now there's a class we call an "API" that cannot be >> generically >> > extended for the sake of saying "we have an API". >> > >> > ---- >> > >> > I wanted to avoid having a drawn out discussion on a jira, where folks >> my >> > not notice it. Especially with things being late in 1.6.0 development >> and >> > the potential this has to impact the API. >> > >> > Personally, I don't have much of a dog in the fight. There's always some >> > arbitrary line for where the public API will be, presuming we want to >> have >> > any kind of balance between providing a stable based for others to >> build on >> > and being able to refactor things. I would like us to hold to our API >> > promises[2] and I would rather we not leak implementation details >> > unnecessarily. >> > >> > I suspect the choice to make MiniAccumuloCluster a class rather than an >> > interface with a factory was driven by the restrictions we put on API >> > changes between major versions and the fact that 1.5 had a class you >> could >> > instantiate via constructors[3]. >> > >> > It's possible we can address some of the major reusability concerns by >> > moving most of the implementation back into MAC, liberally using package >> > access for members, and making the internal-use MAC extend the public >> one. >> > >> > >> > [1]: https://issues.apache.org/jira/browse/ACCUMULO-2143 >> > [2]: http://accumulo.apache.org/governance/releasing.html >> > [3]: https://issues.apache.org/jira/browse/ACCUMULO-2151 >> > >> > >
