zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Han <h...@cloudera.com>
Subject Re: reconfig APIs
Date Sun, 05 Feb 2017 06:43:05 GMT
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 <hanm@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 <
> jordan@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 <hanm@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 <phunt@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 <
>> >> jordan@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 <phunt@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" <
>> jordan@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 <phunt@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 <
>> >>>>> jordan@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 <phunt@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
<
>> >>>>>>> jordan@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
<
>> >>>>>>>>> jordan@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.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message