On Wed, Dec 6, 2017 at 11:56 AM, Josh Elser <els...@apache.org> wrote: > Maybe a difference in interpretation: > > I was seeing 1a as being source-compatible still. My assumption was that > "Deprecate ClientConfiguration" meant that it would remain in the codebase > -- "replace" as in "replace expected user invocation", not removal of the > old ClientConfiguration and addition of a new ClientConfig class.
Ok, if we deprecate ClientConfiguration, leave it in 2.0, and drop the extends from ClientConfiguration in 2.0. Then I am not sure what the benefit of introducing the new ClientConfig type is? > > > On 12/6/17 11:29 AM, Keith Turner wrote: >> >> On Wed, Dec 6, 2017 at 11:28 AM, Josh Elser <els...@apache.org> wrote: >>> >>> 1.a sounds better to me. >> >> >> why? >> >>> >>> A would be the ideal solution, I think B is the next best if A doesn't >>> work. >>> >>> I need to get the Hadoop3 compatibility fixed, so I'll be investigating >>> the >>> Hadoop shaded artifacts this week. >>> >>> >>> On 12/5/17 6:43 PM, Christopher wrote: >>>> >>>> >>>> I was wondering about Hadoop 3 shading and whether that would help us. >>>> It >>>> would be really nice if it could, or if there was some other class path >>>> solution that was easy. >>>> >>>> I think there are two major issues in this thread. The first is the API >>>> problems. The second is the Hadoop 3 support. They are related, but I >>>> think >>>> quickly dealing with the API issues can clarify what our options are for >>>> Hadoop 3. >>>> >>>> >>>> >>>> >>>> To fix the API, I would like to get consensus on proceeding with this >>>> path: >>>> >>>> 1. Rename 1.8.2-SNAPSHOT to 1.9.0-SNAPSHOT and deprecate the existing >>>> ZooKeeperInstance constructor which takes a Configuration >>>> a) Deprecate ClientConfiguration and replace with ClientConfig (or >>>> a >>>> better name) which does not extend Configuration or have API leak >>>> problems, >>>> and add a new ZKI constructor for this >>>> b) Ignore extends for now, and drop it from ClientConfiguration in >>>> 2.0 >>>> with a break (can't deprecate superclass), and add new ZKI constructor >>>> for >>>> more specific ClientConfiguration next to deprecated one >>>> 2. Drop deprecated stuff from 2.0 branch (and extends, if option 1.b was >>>> chosen) >>>> 3. Plan a 1.9.0 release instead of 1.8.2 >>>> >>>> I prefer 1.a over 1.b, personally, but I've been tossing back and forth. >>>> I >>>> would need input on which is best. There are pros and cons to both, >>>> regarding churn, and source and binary compatibility. >>>> >>>> >>>> >>>> >>>> Once we deal with the API, our options for Hadoop 3 become: >>>> >>>> A. Use Hadoop 3 shaded artifacts or some other class path solution (such >>>> as >>>> getting lucky identifying a version of commons-beanutils that works for >>>> both) >>>> B. Shade in 1.9 with a breaking change >>>> C. Create a 1.9 version named 2.0, so we can do a breaking change >>>> without >>>> semver violation; shade in this version >>>> D. Shade in the branch we're currently calling 2.0 >>>> >>>> I think we can defer that decision pending some further >>>> investigation/experimentation into what works, and deal with it after >>>> dealing with steps 1-3 above (but soon after, hopefully). >>>> >>>> >>>> >>>> On Tue, Dec 5, 2017 at 3:58 PM Josh Elser <els...@apache.org> wrote: >>>> >>>>> Another potential suggestion I forgot about: we try to just move to the >>>>> Hadoop shaded artifacts. This would invalidate the need to do more, but >>>>> I have no idea how "battle-tested" those artifacts are. >>>>> >>>>> On 12/5/17 3:52 PM, Keith Turner wrote: >>>>>> >>>>>> >>>>>> If we do the following. >>>>>> >>>>>> * Drop ZooKeeperInstance.ZooKeeperInstance(Configuration config) >>>>> >>>>> >>>>> method. >>>>>> >>>>>> >>>>>> * Drop extends from ClientConfig >>>>>> * Add a method ZooKeeperInstance.ZooKeeperInstance(ClientConfig >>>>>> config) >>>>>> >>>>>> Then this will not be binary compatible, so it will still be painful >>>>>> in many cases. It may be source compatible. >>>>>> >>>>>> For example the following will be source (but not binary) compatible. >>>>>> >>>>>> ClientConfiguration cc = new ClientConfiguration(file); >>>>>> //when compiled against older version of Accumulo will bind to >>>>>> method with commons config signature >>>>>> //when recompiled will bind to clientconfig version of method >>>>>> ZooKeeperInstance zki = new ZooKeeperInstance(cc); >>>>>> >>>>>> The following would not be source or binary compatible. >>>>>> >>>>>> Configuration cc = new ClientConfiguration(file); >>>>>> ZooKeeperInstance zki = new ZooKeeperInstance(cc); >>>>>> >>>>>> >>>>>> On Tue, Dec 5, 2017 at 3:40 PM, Josh Elser <els...@apache.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 12/5/17 3:28 PM, Keith Turner wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Dec 5, 2017 at 2:53 PM, Josh Elser<els...@apache.org> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Interesting. What makes you want to deprecate ClientConfig >>>>>>>>> entirely? >>>>>>>>> >>>>>>>>> I'd be worried about removing without sufficient thought of >>>>> >>>>> >>>>> replacement >>>>>>>>> >>>>>>>>> >>>>>>>>> around. It would be a bit "churn-y" to introduce yet another way >>>>>>>>> that >>>>>>>>> clients have to connect (since it was introduced in 1.6-ish?). >>>>>>>>> Working >>>>>>>>> around the ClientConfig changes was irritating for the downstream >>>>>>>>> integrations (Hive, most notably). >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Ok maybe thats a bad idea, not looking to cause pain. Here were >>>>>>>> some >>>>>>>> of my goals. >>>>>>>> >>>>>>>> * Remove commons config from API completely via deprecation >>>>>>>> cycle. >>>>>>>> * Introduce API that supports putting all props needed to >>>>>>>> connect >>>>>>>> to >>>>>>>> Accumulo in an API. >>>>>>>> >>>>>>>> I suppose if we want to keep ClientConfig class in API, then there >>>>>>>> is >>>>>>>> no way to remove commons config via a deprecation cycle?? We can't >>>>>>>> deprecate the extension of commons config, all we can do is just >>>>>>>> drop >>>>>>>> it at some point. >>>>>>>> >>>>>>> >>>>>>> My line of thinking is that the majority of the time, we're creating >>>>>>> a >>>>>>> ClientConfiguration by one of: >>>>>>> >>>>>>> * ClientConfiguration#loadDefault() >>>>>>> * new ClientConfiguration(String) >>>>>>> * new ClientConfiguration(File) >>>>>>> >>>>>>> Granted, we also inherit/expose a few other things (notably extending >>>>>>> CompositeConfiguration and throwing ConfigurationException). I would >>>>>>> be >>>>>>> comfortable with dropping those w/o deprecation. I have not seen >>>>> >>>>> >>>>> evidence >>>>>>> >>>>>>> >>>>>>> from anyone that they are widely in use by folks (although I've not >>>>>>> explicitly asked, either). >>>>> >>>>> >>>>> >>>> >>> >
