We now have two pull requests pending for this issue per discussion in https://github.com/apache/zookeeper/pull/122
The PR for master branch is now: #152 <https://github.com/apache/zookeeper/pull/152> The PR for branch-3.5 is now: #151 <https://github.com/apache/zookeeper/pull/151> I've tested both pull requests locally and everything looks good. Please have a look if anyone has any cycles next week - I plan to merge both by end of next week (2/10) so we can get 3.5.3 release moving forward. On Thu, Jan 5, 2017 at 12:17 PM, Michael Han <h...@cloudera.com> wrote: > Patch looking good for ZOOKEEPER-2642 (https://github.com/apache/ > zookeeper/pull/122). > > I just tested locally with both stress unit tests and zkCli.sh, everything > looks fine. If anyone has any cycles could you please review / test this > patch? I'd like to have more +1s before getting this in - and it would be > good if we can get this in sooner so we can cut out a release candidate > build for 3.5.3 (beta). > > On Thu, Dec 8, 2016 at 2:50 PM, Jordan Zimmerman < > jor...@jordanzimmerman.com> wrote: > >> Thanks for the replies everyone. Adding the methods back with a >> deprecation notice solves Curator’s problems so I’ll proceed with that on >> ZOOKEEPER-2642. >> >> -Jordan >> >> > On Dec 8, 2016, at 6:56 PM, Michael Han <h...@cloudera.com> wrote: >> > >> > Thanks Jordan for raising the concern. It's a reasonable concern with >> good >> > intention to make user's life easier, which is important to the project. >> > >> > There are two separate issues we need to reach a consensus here: >> > >> > * Move reconfig API >> > The details of why the decision was made is documented in ZOOKEEPER-2014 >> > JIRA, but just to recap, the API is moved because it's the right thing >> to >> > do in long term, which yield a cleaner interface design for ZooKeeper >> > client API. In future, we might also want to clean up other APIs (i.e. >> move >> > quota related APIs over as well.) as well. While seemingly stylish as it >> > might look like, I believe a clean API is also important for a project's >> > usability / adoption. From the discussions so far I think everyone >> either >> > agree with this, or have no strong opinion against it, so I consider >> this >> > question is solved. >> > >> > * Backward compatibility between releases >> > Since we decide refactor reconfig API, the question is when. The >> reconfig >> > API was removed from ZOOKEEPER-2014 (which targets 3.5.3 alpha) because >> of >> > the definition of alpha / beta releases in the context of ZooKeeper >> project >> > (I'm not saying this is right or wrong, simply state a fact up to >> date): an >> > alpha release implies that we allow backward incompatible changes, and a >> > beta release implies that the APIs are locked down and none backward >> > compatible change. Given this definition, it is perfectly legitimate to >> > move API around w/o considering compatibility. Now what Jordan pointed >> out >> > regarding definition of 'alpha' combined with some old discussion >> threads I >> > found in user list indicate that our definition of 'alpha', and 'beta' >> is >> > none standard and could cause confusions. So I think we should try >> reach a >> > consensus on whether we stick with current definition of 'alpha' and >> 'beta' >> > release during 3.5.3 release cycle, or adopt something else. >> > >> > With all these said, I think options are: >> > * Do nothing, stick with current alpha / beta definition. >> > * Try redefine what 'alpha' and 'beta' means in the context of ZooKeeper >> > and then go from there in 3.5.3 release cycle. >> > >> > In any cases, I'd like to propose we keep the reconfig API in >> > ZooKeeperAdmin which as previously stated is the right thing to do in >> long >> > term. If we decide to maintain backward API compatibility for 3.5.3-beta >> > release, we could add reconfig API back to ZooKeeper but mark it as >> > deprecated, then remove the API in next major release (3.6 or 4.x). >> > >> > >> > On Thu, Dec 8, 2016 at 8:27 AM, Patrick Hunt <ph...@apache.org> wrote: >> > >> >> I've only seen a few questions about versioning come up on the ZK >> lists, >> >> they were quickly responded to with pointers to the process. iirc we >> based >> >> our versioning scheme on what Hadoop was/is using. Additionally if we >> don't >> >> allow for alpha time it will further slow progress as there will be no >> >> opportunity to adjust things like APIs once they are committed and >> >> generally available for people to "kick the tires". >> >> >> >> I'll leave it up to the rest of the community to state their opinions, >> but >> >> as i have stated imo it would be a mistake to revert this change. >> Jordan >> >> has raised a reasonable concern - I consider this a blocker for >> 3.5.3-alpha >> >> until it is resolved. >> >> >> >> Patrick >> >> >> >> On Thu, Dec 8, 2016 at 1:46 AM, Jordan Zimmerman < >> >> jor...@jordanzimmerman.com >> >>> wrote: >> >> >> >>>> I think people understand what alpha means. >> >>> >> >>> With respect, the ZooKeeper team has used “alpha” in a novel way that >> is >> >>> sowing considerable confusion. I wrote emails about this a while back. >> >> But, >> >>> here again, is another case where the non-standard usage of “alpha” >> will >> >>> cause confusion. To reiterate: someone who sees "3.5.2-alpha” will >> think >> >>> that there will eventually be a “3.5.2-beta” and finally a “3.5.2” >> >> release >> >>> version. But, with ZooKeeper there will never be a “3.5.2” release. In >> >>> fact, the “-alpha” label is mis-located. The real version is >> >>> “3.5.?-alpha1”, “3.5.?-alpha2”, etc. There’s an important implication >> >> here. >> >>> If a ZooKeeper user who doesn’t follow the mailing lists, etc. sees >> >>> “3.5.2-alpha” and “3.5.3-alpha” they will mentally see these as two >> >>> different releases. What ZOOKEEPER-2014 has done is remove an existing >> >> API >> >>> from what appears to be a released version 3.5.2 (which never really >> >>> existed). This change violates semantic versioning. For external >> users, >> >> the >> >>> version after “3.5.2” should be “4.x.x” as it has breaking changes. >> >>> >> >>>> It's not about style, there were a number of concerns addressed in >> that >> >>>> patch. >> >>> >> >>> The auth issues are very real ones. The issues in ZOOKEEPER-2014 can >> be >> >>> addressed completely without moving the reconfig() methods to a new >> >> class. >> >>> It may be useful to move APIs around for clarity, etc. but these >> breaking >> >>> changes should be for a version that signals breaking changes - 4.x.x >> or >> >>> something. i.e. moving the reconfig() APIs is orthogonal to the >> concerns >> >> of >> >>> ZOOKEEPER-2014 and should be a separate Jira issue. >> >>> >> >>>> I think people understand what alpha means. There may be some short >> >> term >> >>>> impact for a few, but a significant benefit over the long term. >> >>> >> >>> Again with respect - 3.5.0-alpha was made available in Maven Central >> >>> August 2014 - over 2 years ago. Regardless of the ZooKeeper team’s >> >> intent, >> >>> this is NOT an alpha in users’ minds. This is a released library that >> is >> >> in >> >>> production in major organizations. The ZooKeeper team should realize >> this >> >>> and adjust their thinking about the “alpha” tag. For Apache Curator, >> >> we’re >> >>> now put in a position where the reconfig() APIs have disappeared. In >> >> order >> >>> to maintain our own internal semantic versioning we will have to >> >> consider a >> >>> third branch of Curator (we currently have a ZooKeeper 3.4.x >> compatible >> >>> version and a ZooKeeper 3.5.x compatible version). It would be >> awesome if >> >>> we didn’t have to do this. >> >>> >> >>> -Jordan >> >>> >> >>>> On Dec 7, 2016, at 7:16 PM, Patrick Hunt <ph...@apache.org> wrote: >> >>>> >> >>>> It's not about style, there were a number of concerns addressed in >> that >> >>>> patch. We didn't take the change lightly, we've been discussing it >> over >> >>>> jira and the mailing list over the past two years. >> >>>> >> >>>> I think people understand what alpha means. There may be some short >> >> term >> >>>> impact for a few, but a significant benefit over the long term. >> >>>> >> >>>> Patrick >> >>>> >> >>>> On Dec 7, 2016 9:24 AM, "Jordan Zimmerman" < >> jor...@jordanzimmerman.com >> >>> >> >>>> wrote: >> >>>> >> >>>>> I read through the issue and disagree about the decision to move the >> >>> APIs >> >>>>> out. That was a stylistic choice that breaks client code. I realize >> >> that >> >>>>> 3.5.x has been advertised as an alpha but you must realize that most >> >> of >> >>> the >> >>>>> world is using it in production. These APIs have now been published. >> >>> This >> >>>>> will create a real headache for Curator which is why I’ve created >> >>>>> https://issues.apache.org/jira/browse/ZOOKEEPER-2642 < >> >>>>> https://issues.apache.org/jira/browse/ZOOKEEPER-2642> - I hope we >> can >> >>>>> move these APIs back into ZooKeeper.java. >> >>>>> >> >>>>> -Jordan >> >>>>> >> >>>>>> On Dec 7, 2016, at 5:54 PM, Patrick Hunt <ph...@apache.org> wrote: >> >>>>>> >> >>>>>> It's discussed in more depth in the JIRA iirc, but basically; >> >>>>>> ZooKeeper.java provides client APIs, reconfig is an admiistrative >> >> API. >> >>>>>> >> >>>>>> Patrick >> >>>>>> >> >>>>>> On Wed, Dec 7, 2016 at 8:48 AM, Jordan Zimmerman < >> >>>>> jor...@jordanzimmerman.com >> >>>>>>> wrote: >> >>>>>> >> >>>>>>> I understand the need to make the methods require proper auth but >> >>>>> there's >> >>>>>>> no reason to move it to a different class that I can see. Am I >> >> missing >> >>>>>>> something? >> >>>>>>> >> >>>>>>> ==================== >> >>>>>>> Jordan Zimmerman >> >>>>>>> >> >>>>>>>> On Dec 7, 2016, at 4:37 PM, Patrick Hunt <ph...@apache.org> >> wrote: >> >>>>>>>> >> >>>>>>>> This problem has been a long standing blocker issue for 3.5 and >> >>>>>>> identified >> >>>>>>>> early on as something that would need to change. This has been >> one >> >> of >> >>>>> the >> >>>>>>>> reasons why 3.5 has stayed in alpha - because we allow >> non-backward >> >>>>>>>> compatible changes to new APIs in alpha and we knew we would have >> >> to >> >>>>> fix >> >>>>>>>> this. The description/comments of ZOOKEEPER-2014 does a good job >> >>>>>>>> documenting why this had to change. >> >>>>>>>> >> >>>>>>>> Patrick >> >>>>>>>> >> >>>>>>>> On Wed, Dec 7, 2016 at 3:18 AM, Jordan Zimmerman < >> >>>>>>> jor...@jordanzimmerman.com >> >>>>>>>>> wrote: >> >>>>>>>> >> >>>>>>>>> OK - I found the offending issue: ZOOKEEPER-2014 >> >>>>>>>>> >> >>>>>>>>> What is the benefit/logic of moving the reconfig() variants >> into a >> >>> new >> >>>>>>>>> class? I can see if this was done from the start but you have >> now >> >>>>> broken >> >>>>>>>>> Curator in a fairly serious non-backward compatible way for a >> >> minor >> >>>>>>>>> documenting benefit. Does anyone object to me reversing this? >> >>>>>>>>> >> >>>>>>>>> -Jordan >> >>>>>>>>> >> >>>>>>>>>> On Dec 7, 2016, at 11:37 AM, Jordan Zimmerman < >> >>>>>>>>> jor...@jordanzimmerman.com> wrote: >> >>>>>>>>>> >> >>>>>>>>>> Hi, >> >>>>>>>>>> >> >>>>>>>>>> I was compiling Curator against the ZK master and noticed that >> >> the >> >>>>>>>>> reconfig APIs are gone/changed. Can anyone point me at the >> issues >> >>> for >> >>>>>>> this >> >>>>>>>>> and/or the discussion why this breaking change was made? >> >>>>>>>>>> >> >>>>>>>>>> -Jordan >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>> >> >>>>> >> >>>>> >> >>> >> >>> >> >> >> > >> > >> > >> > -- >> > Cheers >> > Michael. >> >> > > > -- > Cheers > Michael. > -- Cheers Michael.
