You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Enrico Olivelli <eo...@gmail.com> on 2017/07/24 12:22:44 UTC
EnsemblePlacementPolicy Public API breaking changes
Hi,
we are going to release 4.5 soon, in this release we changed
EnsemblePlacementPolicy interface.
It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you just
switch the bookeeeper-server JAR on the classpath your application won't
run.
I already got that problem and I needed to implement some "support 4.5"
option in my projects which essentially tells "do not use a custom policy",
this is very bad because there is no way to apply the custom rules required
by the project.
As we are going to break the API now, would it be good to create a more
future-proof API ?
We can just post-pone the problem to 4.6
Thoughts ?
Enrico
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
The end of the story is that discussing with Sijie we decided to close the
issue as won't fix.
In the future we will define the rules to handle backward compatibility.
Changes on the PlacementPolicy are about semantics from 4.4 to 4.5
Enrico
Il mer 26 lug 2017, 12:26 Enrico Olivelli <eo...@gmail.com> ha scritto:
> This is the PR I have tried to address the issue with method overloading
> and interface default methods.
> https://github.com/apache/bookkeeper/pull/301
>
> For my projects it is working
>
> We can defer to 4.6 eventually a complete refactor of this interface
>
> -- Enrico
>
>
>
> 2017-07-25 19:29 GMT+02:00 Enrico Olivelli <eo...@gmail.com>:
>
>>
>>
>> Il mar 25 lug 2017, 19:08 Sijie Guo <gu...@gmail.com> ha scritto:
>>
>>> On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <eo...@gmail.com>
>>> wrote:
>>>
>>> > Il mar 25 lug 2017, 18:12 Sijie Guo <gu...@gmail.com> ha scritto:
>>> >
>>> > > Let me try to summarize the comments here (also as a reference for
>>> other
>>> > > people). This basically is comprised of two questions:
>>> > >
>>> > > 1) how do we handle the 'binary backward compatibility' for drop-in
>>> > upgrade
>>> > > in applications using only one bk version?
>>> > >
>>> > > ideally if we change any method signatures in public API, it is a
>>> > breaking
>>> > > change. we should do this kind of change in a major version release.
>>> > >
>>> > > 4.5 is sort of a major version release because it attempts to merge
>>> > changes
>>> > > accumulated for years from multiple branches. Ideally we should bump
>>> the
>>> > > release version to 5.0. but since it is already close to the release
>>> > date,
>>> > > I will prefer stick the version number to 4.5 but mark it as a major
>>> > > release includes public interface changes (e.g. netty 4 upgrade,
>>> bytebuf
>>> > > introduced, ensemble placement policy interface changed).
>>> > >
>>> > > method overloading should be able keep such binary backward
>>> compatibility
>>> > > in general.
>>> > >
>>> > > 2) how do we handle the 'binary backward compatibility' for drop-in
>>> > upgrade
>>> > > in applications using multiple bk versions?
>>> > >
>>> > > This is tricky because it is really out of the scope of a single
>>> project.
>>> > > It is typically a problem of JVM loading jars. Shading is more a
>>> solution
>>> > > for 2).
>>> > >
>>> > >
>>> > > If your problem is 1), method overloading should be able to take
>>> care of
>>> > > it. You mentioned you tried method overloading, do you mind pointing
>>> me
>>> > > your code change.
>>> > > If your problem is 2), I do not see an easy way to address it even
>>> with
>>> > > your proposal.
>>> > >
>>> > >
>>> > > Other comments inline:
>>> > >
>>> > >
>>> > > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <
>>> eolivelli@gmail.com>
>>> > > wrote:
>>> > >
>>> > > > 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
>>> > > >
>>> > > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
>>> > eolivelli@gmail.com>
>>> > > > > wrote:
>>> > > > >
>>> > > > > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha
>>> > scritto:
>>> > > > > >
>>> > > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
>>> > > > eolivelli@gmail.com>
>>> > > > > > > wrote:
>>> > > > > > >
>>> > > > > > > > Hi,
>>> > > > > > > > we are going to release 4.5 soon, in this release we
>>> changed
>>> > > > > > > > EnsemblePlacementPolicy interface.
>>> > > > > > > >
>>> > > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4
>>> and
>>> > > you
>>> > > > > just
>>> > > > > > > > switch the bookeeeper-server JAR on the classpath your
>>> > > application
>>> > > > > > won't
>>> > > > > > > > run.
>>> > > > > > > >
>>> > > > > > > > I already got that problem and I needed to implement some
>>> > > "support
>>> > > > > 4.5"
>>> > > > > > > > option in my projects which essentially tells "do not use a
>>> > > custom
>>> > > > > > > policy",
>>> > > > > > > > this is very bad because there is no way to apply the
>>> custom
>>> > > rules
>>> > > > > > > required
>>> > > > > > > > by the project.
>>> > > > > > > >
>>> > > > > > >
>>> > > > > > > Do you mean the new methods introduced in placement policy?
>>> Or
>>> > > > methods
>>> > > > > > that
>>> > > > > > > whose signatures are changed?
>>> > > > > > >
>>> > > > > >
>>> > > > > > The new signatures
>>> > > > > >
>>> > > > > > >
>>> > > > > > > I believe the new methods 'reorderSequence' is disabled by
>>> > default
>>> > > > > unless
>>> > > > > > > you enable it explicitly.
>>> > > > > > >
>>> > > > > > > For methods that whose signatures were changed, we can add
>>> the
>>> > old
>>> > > > > > methods
>>> > > > > > > back and create overrides to keep the binary backward
>>> compatible.
>>> > > > > > >
>>> > > > > >
>>> > > > > > I don't know if this cam work. I have already tried. I will
>>> double
>>> > > > check.
>>> > > > > > Anyway it will be a bit messy
>>> > > > > >
>>> > > > > > >
>>> > > > > > > However I am not sure if this works because the placement
>>> policy
>>> > is
>>> > > > > > > instantiated and loaded by reflection. Typically when you
>>> > upgrade a
>>> > > > > > > version, you have to compile it first, no?
>>> > > > > > >
>>> > > > > >
>>> > > > > > I have several libraries which use bk and they are built on
>>> 4.4 and
>>> > > > they
>>> > > > > > are working together in the same classpath.
>>> > > > > > For instance now I am going to change one of them in order to
>>> > > leverage
>>> > > > a
>>> > > > > > new 4.5 feature, like readUnconfirmedEntries just as an
>>> example,
>>> > so
>>> > > I
>>> > > > > need
>>> > > > > > to switch to 4.5 on the classpath but the other projects won't
>>> run.
>>> > > > > >
>>> > > > >
>>> > > > > Are you talking about mixing 4.4 and 4.5 together?
>>> > > > >
>>> > > >
>>> > > > Yes, as for most widely used libraries (like ZooKeeper) it is
>>> common to
>>> > > put
>>> > > > a "new version" or the library on a application compiled for a
>>> previous
>>> > > > version.
>>> > > > I see that sometimes it is not possible, and for this reason we use
>>> > > 'major
>>> > > > versions' vs 'minor versions'.
>>> > > > When possible I think it is best not to introduce such breaking
>>> > changes.
>>> > > >
>>> > >
>>> > > I agree with you. That's the purpose of 'major version' and 'minor
>>> > > version'. However,
>>> > > 4.5 is a special case which it was an attempt to merge multiple
>>> branches
>>> > > together.
>>> > > Although it doesn't break the protocol backward compatibility and
>>> code
>>> > > compatibility
>>> > > in public client API, it does change a bit on some interfaces (for
>>> > example
>>> > > the placement policy interface).
>>> > >
>>> >
>>> > Yes, we did it for the AuthPlugin API too.
>>> >
>>> >
>>> > >
>>> > > >
>>> > > >
>>> > > >
>>> > > >
>>> > > >
>>> > > > >
>>> > > > >
>>> > > > > > For the server side this is not a problem but on the client it
>>> is a
>>> > > > real
>>> > > > > > production problem.
>>> > > > > >
>>> > > > >
>>> > > > > Can you clarify more specifically on this? Not very sure I
>>> understand
>>> > > > your
>>> > > > > problem and what are you going to achieve.
>>> > > > >
>>> > > > >
>>> > > > Usually you have full control on bookies, but it is not always
>>> possible
>>> > > to
>>> > > > have full control of applications assembler from different
>>> components.
>>> > > > Example:
>>> > > > - I have Lib 1 v1 which is compiled with BK 4.4
>>> > > > - I have Lib 2 v1 which is compiled with BK 4.4
>>> > > > - I have App which depends on Lib1 + Lib2
>>> > > > - Now Lib 2 v2 need BK 4.5 for a fresh new feature
>>> > > > - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and
>>> will
>>> > > > need to include BK 4.5, but this won't work
>>> > > > Assemblers for App v2 do not have control on Lib1 and Lib2
>>> > > >
>>> > >
>>> > > I am not sure if we should really deal with mixed version case. If
>>> you
>>> > have
>>> > > multiple version cases, you
>>> > > might prefer shading the dependencies. Newer version of the library
>>> tends
>>> > > to add new methods for new
>>> > > features. If the class loader loads a class with old version, it will
>>> > > anyway break your application using newer
>>> > > library.
>>> > >
>>> > > Your question here is more about "How can we do better
>>> > > to handle the use case of using multiple different versions of
>>> library in
>>> > > same JVM".
>>> > >
>>> > >
>>> > > >
>>> > > > For instance I got into this trouble while integrating my new
>>> project
>>> > > which
>>> > > > uses readUnconfirmedEntries inside an App which is compiled for BK
>>> 4.4
>>> > > and
>>> > > > uses a custom placement policy.
>>> > > >
>>> > > > So, now that we are breaking things I would like to break them in
>>> a way
>>> > > > that in the future will be simpler to handle
>>> > > >
>>> > > > for instance:
>>> > > >
>>> > > > public interface EnsemblePlacementPolicy {
>>> > > >
>>> > > >
>>> > > > public EnsemblePlacementPolicy initialize(ClientEnvironment
>>> env);
>>> > > >
>>> > > > public void uninitalize();
>>> > > >
>>> > > > public Set<BookieSocketAddress>
>>> > > > onClusterChanged(Set<BookieSocketAddress> writableBookies,
>>> > > >
>>> > > > Set<BookieSocketAddress> readOnlyBookies);
>>> > > >
>>> > > > public List<BookieSocketAddress> newEnsemble(LedgerSpecs
>>> > ledgerSpecs,
>>> > > > Set<BookieSocketAddress> excludeBookies) throws
>>> > > > BKNotEnoughBookiesException;
>>> > > >
>>> > > >
>>> > > > public BookieSocketAddress replaceBookie(LedgerSpecs
>>> ledgerSpecs,
>>> > > > Collection<BookieSocketAddress> currentEnsemble,
>>> > > > BookieSocketAddress bookieToReplace,
>>> Set<BookieSocketAddress>
>>> > > > excludeBookies) throws BKNotEnoughBookiesException;
>>> > > >
>>> > > > public List<Integer>
>>> reorderReadSequence(List<BookieSocketAddress>
>>> > > > ensemble,
>>> > > > List<Integer>
>>> writeSet,
>>> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
>>> > > >
>>> > > >
>>> > > > public List<Integer> reorderReadLACSequence(List<
>>> > BookieSocketAddress>
>>> > > > ensemble,
>>> > > > List<Integer>
>>> writeSet,
>>> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
>>> > > >
>>> > > > default public void updateBookieInfo(Map<BookieSocketAddress,
>>> > > > BookieInfo> bookieInfoMap) {
>>> > > > }
>>> > > > }
>>> > > >
>>> > > >
>>> > > > where ClientEnvironment and LedgerSpecs contain all of the data
>>> which
>>> > > need
>>> > > > to be passed to the policy.
>>> > > > We can do even more better and group the other parameters
>>> > > >
>>> > > > we can evaluate to switch to an abstract class, or provide an
>>> official
>>> > > > "Adapter", but DefaultEnsemblePlacementPolicy is already a good
>>> "base"
>>> > > >
>>> > >
>>> > >
>>> > > wrapping fields into classes has same issues with adding fields to
>>> method
>>> > > signature. there isn't really difference when multiple version of
>>> jars
>>> > are
>>> > > in the classpath.
>>> > >
>>> >
>>> > It will let us add more contextual data without breaking the
>>> signatures.
>>> > Old code won't use new facilities and will work.
>>> > In general methods with manu parameters are not a good practice and
>>> many
>>> > overloaded version of the methid become easily tricky to maintain
>>> >
>>>
>>> Enrico, new code will be break if old jar is in the method. that's what I
>>> am trying to explain it to you - on the multiple version cases.
>>>
>>
>> Sure. For 4.5 we have to way to work around and it is better ti break
>> compatibility. I only would like to prepare for the future and do the bes
>>
>>
>>> Both approaches have good and bad parts. So it is a preference and
>>> tradeoff.
>>>
>>
>> Yup
>>
>>>
>>>
>>> >
>>> >
>>> > > so it is really interesting for me to see why doesn't method
>>> overloading
>>> > > address the code binary backward compatibility issue.
>>> > >
>>> >
>>> > I will share some code soon
>>> >
>>> > Another blocker issue is that We should not use Guava in 'public' API
>>> > methods, as we are doing.
>>> > This will be easily resolved, we are usong Optional. I will send PR
>>> soon
>>> >
>>> > >
>>> > >
>>> > >
>>> > > >
>>> > > >
>>> > > >
>>> > > > >
>>> > > > >
>>> > > > > > I am ok with changing the API, I was the first to ask to
>>> change it
>>> > in
>>> > > > 4.5
>>> > > > > > in order to access custom metadata, but when we do such things
>>> I
>>> > > would
>>> > > > > like
>>> > > > > > to design a future proof API.
>>> > > > > >
>>> > > > > > I know it needs some time to design and I really to not want to
>>> > block
>>> > > > 4.5
>>> > > > > > release.
>>> > > > > > I will send a BP and maybe we can discuss on a concrete
>>> proposal.
>>> > > > > >
>>> > > > >
>>> > > > > Can we first make things clear here before any proposals?
>>> > > > >
>>> > > >
>>> > > > yep, sure.
>>> > > > I meant that writing all in a Wiki page makes it simpler for
>>> reviews
>>> > and
>>> > > > comments, in Kafka now they works this way: create KIP ->
>>> discussion ->
>>> > > > adjustments -> vote
>>> > > > this is another problem, to be discussed in another email thraed.
>>> > > >
>>> > >
>>> > > yes. but you already started an email thread to discuss this, no?
>>> > >
>>> > >
>>> > > >
>>> > > > >
>>> > > > >
>>> > > > > > I think this change will break DL too.
>>> > > > > >
>>> > > > >
>>> > > > > When we upgraded the BK in DL, we will make some code changes to
>>> make
>>> > > > sure
>>> > > > > binary compatible.
>>> > > > >
>>> > > >
>>> > > >
>>> > > > I have referred to DL because it seems to me that it is explicit
>>> in DL
>>> > > > documentation that you can provide your own PlacementPolicy, so
>>> users
>>> > > which
>>> > > > implemented their own policy in DL will need to recompile.
>>> > > > In this specific case it it not really a problem because DL did not
>>> > work
>>> > > > with BK 4.4 at all, but it was just an example of a third party
>>> library
>>> > > > which depends on BK
>>> > > >
>>> > >
>>> > > I don't think we expose the placement policy directly in DL. We
>>> expose a
>>> > > DNS resolver instead.
>>> > > But it doesn't prevent user write his/her own placement policy. It
>>> might
>>> > > still have the problem
>>> > > you describe here.
>>> > >
>>> > >
>>> > >
>>> > >
>>> > > >
>>> > > >
>>> > > > >
>>> > > > >
>>> > > > > > Does this sound good to you?
>>> > > > > >
>>> > > > > > Enrico
>>> > > > > >
>>> > > > > >
>>> > > > > > >
>>> > > > > > > >
>>> > > > > > > > As we are going to break the API now, would it be good to
>>> > create
>>> > > a
>>> > > > > more
>>> > > > > > > > future-proof API ?
>>> > > > > > >
>>> > > > > > > We can just post-pone the problem to 4.6
>>> > > > > > > >
>>> > > > > > > > Thoughts ?
>>> > > > > > > >
>>> > > > > > > > Enrico
>>> > > > > > > >
>>> > > > > > >
>>> > > > > > --
>>> > > > > >
>>> > > > > >
>>> > > > > > -- Enrico Olivelli
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> > --
>>> >
>>> >
>>> > -- Enrico Olivelli
>>> >
>>>
>> --
>>
>>
>> -- Enrico Olivelli
>>
>
> --
-- Enrico Olivelli
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
This is the PR I have tried to address the issue with method overloading
and interface default methods.
https://github.com/apache/bookkeeper/pull/301
For my projects it is working
We can defer to 4.6 eventually a complete refactor of this interface
-- Enrico
2017-07-25 19:29 GMT+02:00 Enrico Olivelli <eo...@gmail.com>:
>
>
> Il mar 25 lug 2017, 19:08 Sijie Guo <gu...@gmail.com> ha scritto:
>
>> On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <eo...@gmail.com>
>> wrote:
>>
>> > Il mar 25 lug 2017, 18:12 Sijie Guo <gu...@gmail.com> ha scritto:
>> >
>> > > Let me try to summarize the comments here (also as a reference for
>> other
>> > > people). This basically is comprised of two questions:
>> > >
>> > > 1) how do we handle the 'binary backward compatibility' for drop-in
>> > upgrade
>> > > in applications using only one bk version?
>> > >
>> > > ideally if we change any method signatures in public API, it is a
>> > breaking
>> > > change. we should do this kind of change in a major version release.
>> > >
>> > > 4.5 is sort of a major version release because it attempts to merge
>> > changes
>> > > accumulated for years from multiple branches. Ideally we should bump
>> the
>> > > release version to 5.0. but since it is already close to the release
>> > date,
>> > > I will prefer stick the version number to 4.5 but mark it as a major
>> > > release includes public interface changes (e.g. netty 4 upgrade,
>> bytebuf
>> > > introduced, ensemble placement policy interface changed).
>> > >
>> > > method overloading should be able keep such binary backward
>> compatibility
>> > > in general.
>> > >
>> > > 2) how do we handle the 'binary backward compatibility' for drop-in
>> > upgrade
>> > > in applications using multiple bk versions?
>> > >
>> > > This is tricky because it is really out of the scope of a single
>> project.
>> > > It is typically a problem of JVM loading jars. Shading is more a
>> solution
>> > > for 2).
>> > >
>> > >
>> > > If your problem is 1), method overloading should be able to take care
>> of
>> > > it. You mentioned you tried method overloading, do you mind pointing
>> me
>> > > your code change.
>> > > If your problem is 2), I do not see an easy way to address it even
>> with
>> > > your proposal.
>> > >
>> > >
>> > > Other comments inline:
>> > >
>> > >
>> > > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <eolivelli@gmail.com
>> >
>> > > wrote:
>> > >
>> > > > 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
>> > > >
>> > > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
>> > eolivelli@gmail.com>
>> > > > > wrote:
>> > > > >
>> > > > > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha
>> > scritto:
>> > > > > >
>> > > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
>> > > > eolivelli@gmail.com>
>> > > > > > > wrote:
>> > > > > > >
>> > > > > > > > Hi,
>> > > > > > > > we are going to release 4.5 soon, in this release we changed
>> > > > > > > > EnsemblePlacementPolicy interface.
>> > > > > > > >
>> > > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4
>> and
>> > > you
>> > > > > just
>> > > > > > > > switch the bookeeeper-server JAR on the classpath your
>> > > application
>> > > > > > won't
>> > > > > > > > run.
>> > > > > > > >
>> > > > > > > > I already got that problem and I needed to implement some
>> > > "support
>> > > > > 4.5"
>> > > > > > > > option in my projects which essentially tells "do not use a
>> > > custom
>> > > > > > > policy",
>> > > > > > > > this is very bad because there is no way to apply the custom
>> > > rules
>> > > > > > > required
>> > > > > > > > by the project.
>> > > > > > > >
>> > > > > > >
>> > > > > > > Do you mean the new methods introduced in placement policy? Or
>> > > > methods
>> > > > > > that
>> > > > > > > whose signatures are changed?
>> > > > > > >
>> > > > > >
>> > > > > > The new signatures
>> > > > > >
>> > > > > > >
>> > > > > > > I believe the new methods 'reorderSequence' is disabled by
>> > default
>> > > > > unless
>> > > > > > > you enable it explicitly.
>> > > > > > >
>> > > > > > > For methods that whose signatures were changed, we can add the
>> > old
>> > > > > > methods
>> > > > > > > back and create overrides to keep the binary backward
>> compatible.
>> > > > > > >
>> > > > > >
>> > > > > > I don't know if this cam work. I have already tried. I will
>> double
>> > > > check.
>> > > > > > Anyway it will be a bit messy
>> > > > > >
>> > > > > > >
>> > > > > > > However I am not sure if this works because the placement
>> policy
>> > is
>> > > > > > > instantiated and loaded by reflection. Typically when you
>> > upgrade a
>> > > > > > > version, you have to compile it first, no?
>> > > > > > >
>> > > > > >
>> > > > > > I have several libraries which use bk and they are built on 4.4
>> and
>> > > > they
>> > > > > > are working together in the same classpath.
>> > > > > > For instance now I am going to change one of them in order to
>> > > leverage
>> > > > a
>> > > > > > new 4.5 feature, like readUnconfirmedEntries just as an
>> example,
>> > so
>> > > I
>> > > > > need
>> > > > > > to switch to 4.5 on the classpath but the other projects won't
>> run.
>> > > > > >
>> > > > >
>> > > > > Are you talking about mixing 4.4 and 4.5 together?
>> > > > >
>> > > >
>> > > > Yes, as for most widely used libraries (like ZooKeeper) it is
>> common to
>> > > put
>> > > > a "new version" or the library on a application compiled for a
>> previous
>> > > > version.
>> > > > I see that sometimes it is not possible, and for this reason we use
>> > > 'major
>> > > > versions' vs 'minor versions'.
>> > > > When possible I think it is best not to introduce such breaking
>> > changes.
>> > > >
>> > >
>> > > I agree with you. That's the purpose of 'major version' and 'minor
>> > > version'. However,
>> > > 4.5 is a special case which it was an attempt to merge multiple
>> branches
>> > > together.
>> > > Although it doesn't break the protocol backward compatibility and code
>> > > compatibility
>> > > in public client API, it does change a bit on some interfaces (for
>> > example
>> > > the placement policy interface).
>> > >
>> >
>> > Yes, we did it for the AuthPlugin API too.
>> >
>> >
>> > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > > For the server side this is not a problem but on the client it
>> is a
>> > > > real
>> > > > > > production problem.
>> > > > > >
>> > > > >
>> > > > > Can you clarify more specifically on this? Not very sure I
>> understand
>> > > > your
>> > > > > problem and what are you going to achieve.
>> > > > >
>> > > > >
>> > > > Usually you have full control on bookies, but it is not always
>> possible
>> > > to
>> > > > have full control of applications assembler from different
>> components.
>> > > > Example:
>> > > > - I have Lib 1 v1 which is compiled with BK 4.4
>> > > > - I have Lib 2 v1 which is compiled with BK 4.4
>> > > > - I have App which depends on Lib1 + Lib2
>> > > > - Now Lib 2 v2 need BK 4.5 for a fresh new feature
>> > > > - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and
>> will
>> > > > need to include BK 4.5, but this won't work
>> > > > Assemblers for App v2 do not have control on Lib1 and Lib2
>> > > >
>> > >
>> > > I am not sure if we should really deal with mixed version case. If you
>> > have
>> > > multiple version cases, you
>> > > might prefer shading the dependencies. Newer version of the library
>> tends
>> > > to add new methods for new
>> > > features. If the class loader loads a class with old version, it will
>> > > anyway break your application using newer
>> > > library.
>> > >
>> > > Your question here is more about "How can we do better
>> > > to handle the use case of using multiple different versions of
>> library in
>> > > same JVM".
>> > >
>> > >
>> > > >
>> > > > For instance I got into this trouble while integrating my new
>> project
>> > > which
>> > > > uses readUnconfirmedEntries inside an App which is compiled for BK
>> 4.4
>> > > and
>> > > > uses a custom placement policy.
>> > > >
>> > > > So, now that we are breaking things I would like to break them in a
>> way
>> > > > that in the future will be simpler to handle
>> > > >
>> > > > for instance:
>> > > >
>> > > > public interface EnsemblePlacementPolicy {
>> > > >
>> > > >
>> > > > public EnsemblePlacementPolicy initialize(ClientEnvironment
>> env);
>> > > >
>> > > > public void uninitalize();
>> > > >
>> > > > public Set<BookieSocketAddress>
>> > > > onClusterChanged(Set<BookieSocketAddress> writableBookies,
>> > > >
>> > > > Set<BookieSocketAddress> readOnlyBookies);
>> > > >
>> > > > public List<BookieSocketAddress> newEnsemble(LedgerSpecs
>> > ledgerSpecs,
>> > > > Set<BookieSocketAddress> excludeBookies) throws
>> > > > BKNotEnoughBookiesException;
>> > > >
>> > > >
>> > > > public BookieSocketAddress replaceBookie(LedgerSpecs
>> ledgerSpecs,
>> > > > Collection<BookieSocketAddress> currentEnsemble,
>> > > > BookieSocketAddress bookieToReplace,
>> Set<BookieSocketAddress>
>> > > > excludeBookies) throws BKNotEnoughBookiesException;
>> > > >
>> > > > public List<Integer> reorderReadSequence(List<
>> BookieSocketAddress>
>> > > > ensemble,
>> > > > List<Integer> writeSet,
>> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
>> > > >
>> > > >
>> > > > public List<Integer> reorderReadLACSequence(List<
>> > BookieSocketAddress>
>> > > > ensemble,
>> > > > List<Integer>
>> writeSet,
>> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
>> > > >
>> > > > default public void updateBookieInfo(Map<BookieSocketAddress,
>> > > > BookieInfo> bookieInfoMap) {
>> > > > }
>> > > > }
>> > > >
>> > > >
>> > > > where ClientEnvironment and LedgerSpecs contain all of the data
>> which
>> > > need
>> > > > to be passed to the policy.
>> > > > We can do even more better and group the other parameters
>> > > >
>> > > > we can evaluate to switch to an abstract class, or provide an
>> official
>> > > > "Adapter", but DefaultEnsemblePlacementPolicy is already a good
>> "base"
>> > > >
>> > >
>> > >
>> > > wrapping fields into classes has same issues with adding fields to
>> method
>> > > signature. there isn't really difference when multiple version of jars
>> > are
>> > > in the classpath.
>> > >
>> >
>> > It will let us add more contextual data without breaking the signatures.
>> > Old code won't use new facilities and will work.
>> > In general methods with manu parameters are not a good practice and many
>> > overloaded version of the methid become easily tricky to maintain
>> >
>>
>> Enrico, new code will be break if old jar is in the method. that's what I
>> am trying to explain it to you - on the multiple version cases.
>>
>
> Sure. For 4.5 we have to way to work around and it is better ti break
> compatibility. I only would like to prepare for the future and do the bes
>
>
>> Both approaches have good and bad parts. So it is a preference and
>> tradeoff.
>>
>
> Yup
>
>>
>>
>> >
>> >
>> > > so it is really interesting for me to see why doesn't method
>> overloading
>> > > address the code binary backward compatibility issue.
>> > >
>> >
>> > I will share some code soon
>> >
>> > Another blocker issue is that We should not use Guava in 'public' API
>> > methods, as we are doing.
>> > This will be easily resolved, we are usong Optional. I will send PR soon
>> >
>> > >
>> > >
>> > >
>> > > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > > I am ok with changing the API, I was the first to ask to change
>> it
>> > in
>> > > > 4.5
>> > > > > > in order to access custom metadata, but when we do such things I
>> > > would
>> > > > > like
>> > > > > > to design a future proof API.
>> > > > > >
>> > > > > > I know it needs some time to design and I really to not want to
>> > block
>> > > > 4.5
>> > > > > > release.
>> > > > > > I will send a BP and maybe we can discuss on a concrete
>> proposal.
>> > > > > >
>> > > > >
>> > > > > Can we first make things clear here before any proposals?
>> > > > >
>> > > >
>> > > > yep, sure.
>> > > > I meant that writing all in a Wiki page makes it simpler for reviews
>> > and
>> > > > comments, in Kafka now they works this way: create KIP ->
>> discussion ->
>> > > > adjustments -> vote
>> > > > this is another problem, to be discussed in another email thraed.
>> > > >
>> > >
>> > > yes. but you already started an email thread to discuss this, no?
>> > >
>> > >
>> > > >
>> > > > >
>> > > > >
>> > > > > > I think this change will break DL too.
>> > > > > >
>> > > > >
>> > > > > When we upgraded the BK in DL, we will make some code changes to
>> make
>> > > > sure
>> > > > > binary compatible.
>> > > > >
>> > > >
>> > > >
>> > > > I have referred to DL because it seems to me that it is explicit in
>> DL
>> > > > documentation that you can provide your own PlacementPolicy, so
>> users
>> > > which
>> > > > implemented their own policy in DL will need to recompile.
>> > > > In this specific case it it not really a problem because DL did not
>> > work
>> > > > with BK 4.4 at all, but it was just an example of a third party
>> library
>> > > > which depends on BK
>> > > >
>> > >
>> > > I don't think we expose the placement policy directly in DL. We
>> expose a
>> > > DNS resolver instead.
>> > > But it doesn't prevent user write his/her own placement policy. It
>> might
>> > > still have the problem
>> > > you describe here.
>> > >
>> > >
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > > Does this sound good to you?
>> > > > > >
>> > > > > > Enrico
>> > > > > >
>> > > > > >
>> > > > > > >
>> > > > > > > >
>> > > > > > > > As we are going to break the API now, would it be good to
>> > create
>> > > a
>> > > > > more
>> > > > > > > > future-proof API ?
>> > > > > > >
>> > > > > > > We can just post-pone the problem to 4.6
>> > > > > > > >
>> > > > > > > > Thoughts ?
>> > > > > > > >
>> > > > > > > > Enrico
>> > > > > > > >
>> > > > > > >
>> > > > > > --
>> > > > > >
>> > > > > >
>> > > > > > -- Enrico Olivelli
>> > > > > >
>> > > > >
>> > > >
>> > >
>> > --
>> >
>> >
>> > -- Enrico Olivelli
>> >
>>
> --
>
>
> -- Enrico Olivelli
>
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
Il mar 25 lug 2017, 19:08 Sijie Guo <gu...@gmail.com> ha scritto:
> On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <eo...@gmail.com>
> wrote:
>
> > Il mar 25 lug 2017, 18:12 Sijie Guo <gu...@gmail.com> ha scritto:
> >
> > > Let me try to summarize the comments here (also as a reference for
> other
> > > people). This basically is comprised of two questions:
> > >
> > > 1) how do we handle the 'binary backward compatibility' for drop-in
> > upgrade
> > > in applications using only one bk version?
> > >
> > > ideally if we change any method signatures in public API, it is a
> > breaking
> > > change. we should do this kind of change in a major version release.
> > >
> > > 4.5 is sort of a major version release because it attempts to merge
> > changes
> > > accumulated for years from multiple branches. Ideally we should bump
> the
> > > release version to 5.0. but since it is already close to the release
> > date,
> > > I will prefer stick the version number to 4.5 but mark it as a major
> > > release includes public interface changes (e.g. netty 4 upgrade,
> bytebuf
> > > introduced, ensemble placement policy interface changed).
> > >
> > > method overloading should be able keep such binary backward
> compatibility
> > > in general.
> > >
> > > 2) how do we handle the 'binary backward compatibility' for drop-in
> > upgrade
> > > in applications using multiple bk versions?
> > >
> > > This is tricky because it is really out of the scope of a single
> project.
> > > It is typically a problem of JVM loading jars. Shading is more a
> solution
> > > for 2).
> > >
> > >
> > > If your problem is 1), method overloading should be able to take care
> of
> > > it. You mentioned you tried method overloading, do you mind pointing me
> > > your code change.
> > > If your problem is 2), I do not see an easy way to address it even with
> > > your proposal.
> > >
> > >
> > > Other comments inline:
> > >
> > >
> > > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <eo...@gmail.com>
> > > wrote:
> > >
> > > > 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
> > > >
> > > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
> > eolivelli@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha
> > scritto:
> > > > > >
> > > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > > > eolivelli@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > > > EnsemblePlacementPolicy interface.
> > > > > > > >
> > > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4
> and
> > > you
> > > > > just
> > > > > > > > switch the bookeeeper-server JAR on the classpath your
> > > application
> > > > > > won't
> > > > > > > > run.
> > > > > > > >
> > > > > > > > I already got that problem and I needed to implement some
> > > "support
> > > > > 4.5"
> > > > > > > > option in my projects which essentially tells "do not use a
> > > custom
> > > > > > > policy",
> > > > > > > > this is very bad because there is no way to apply the custom
> > > rules
> > > > > > > required
> > > > > > > > by the project.
> > > > > > > >
> > > > > > >
> > > > > > > Do you mean the new methods introduced in placement policy? Or
> > > > methods
> > > > > > that
> > > > > > > whose signatures are changed?
> > > > > > >
> > > > > >
> > > > > > The new signatures
> > > > > >
> > > > > > >
> > > > > > > I believe the new methods 'reorderSequence' is disabled by
> > default
> > > > > unless
> > > > > > > you enable it explicitly.
> > > > > > >
> > > > > > > For methods that whose signatures were changed, we can add the
> > old
> > > > > > methods
> > > > > > > back and create overrides to keep the binary backward
> compatible.
> > > > > > >
> > > > > >
> > > > > > I don't know if this cam work. I have already tried. I will
> double
> > > > check.
> > > > > > Anyway it will be a bit messy
> > > > > >
> > > > > > >
> > > > > > > However I am not sure if this works because the placement
> policy
> > is
> > > > > > > instantiated and loaded by reflection. Typically when you
> > upgrade a
> > > > > > > version, you have to compile it first, no?
> > > > > > >
> > > > > >
> > > > > > I have several libraries which use bk and they are built on 4.4
> and
> > > > they
> > > > > > are working together in the same classpath.
> > > > > > For instance now I am going to change one of them in order to
> > > leverage
> > > > a
> > > > > > new 4.5 feature, like readUnconfirmedEntries just as an example,
> > so
> > > I
> > > > > need
> > > > > > to switch to 4.5 on the classpath but the other projects won't
> run.
> > > > > >
> > > > >
> > > > > Are you talking about mixing 4.4 and 4.5 together?
> > > > >
> > > >
> > > > Yes, as for most widely used libraries (like ZooKeeper) it is common
> to
> > > put
> > > > a "new version" or the library on a application compiled for a
> previous
> > > > version.
> > > > I see that sometimes it is not possible, and for this reason we use
> > > 'major
> > > > versions' vs 'minor versions'.
> > > > When possible I think it is best not to introduce such breaking
> > changes.
> > > >
> > >
> > > I agree with you. That's the purpose of 'major version' and 'minor
> > > version'. However,
> > > 4.5 is a special case which it was an attempt to merge multiple
> branches
> > > together.
> > > Although it doesn't break the protocol backward compatibility and code
> > > compatibility
> > > in public client API, it does change a bit on some interfaces (for
> > example
> > > the placement policy interface).
> > >
> >
> > Yes, we did it for the AuthPlugin API too.
> >
> >
> > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > For the server side this is not a problem but on the client it
> is a
> > > > real
> > > > > > production problem.
> > > > > >
> > > > >
> > > > > Can you clarify more specifically on this? Not very sure I
> understand
> > > > your
> > > > > problem and what are you going to achieve.
> > > > >
> > > > >
> > > > Usually you have full control on bookies, but it is not always
> possible
> > > to
> > > > have full control of applications assembler from different
> components.
> > > > Example:
> > > > - I have Lib 1 v1 which is compiled with BK 4.4
> > > > - I have Lib 2 v1 which is compiled with BK 4.4
> > > > - I have App which depends on Lib1 + Lib2
> > > > - Now Lib 2 v2 need BK 4.5 for a fresh new feature
> > > > - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and
> will
> > > > need to include BK 4.5, but this won't work
> > > > Assemblers for App v2 do not have control on Lib1 and Lib2
> > > >
> > >
> > > I am not sure if we should really deal with mixed version case. If you
> > have
> > > multiple version cases, you
> > > might prefer shading the dependencies. Newer version of the library
> tends
> > > to add new methods for new
> > > features. If the class loader loads a class with old version, it will
> > > anyway break your application using newer
> > > library.
> > >
> > > Your question here is more about "How can we do better
> > > to handle the use case of using multiple different versions of library
> in
> > > same JVM".
> > >
> > >
> > > >
> > > > For instance I got into this trouble while integrating my new project
> > > which
> > > > uses readUnconfirmedEntries inside an App which is compiled for BK
> 4.4
> > > and
> > > > uses a custom placement policy.
> > > >
> > > > So, now that we are breaking things I would like to break them in a
> way
> > > > that in the future will be simpler to handle
> > > >
> > > > for instance:
> > > >
> > > > public interface EnsemblePlacementPolicy {
> > > >
> > > >
> > > > public EnsemblePlacementPolicy initialize(ClientEnvironment env);
> > > >
> > > > public void uninitalize();
> > > >
> > > > public Set<BookieSocketAddress>
> > > > onClusterChanged(Set<BookieSocketAddress> writableBookies,
> > > >
> > > > Set<BookieSocketAddress> readOnlyBookies);
> > > >
> > > > public List<BookieSocketAddress> newEnsemble(LedgerSpecs
> > ledgerSpecs,
> > > > Set<BookieSocketAddress> excludeBookies) throws
> > > > BKNotEnoughBookiesException;
> > > >
> > > >
> > > > public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
> > > > Collection<BookieSocketAddress> currentEnsemble,
> > > > BookieSocketAddress bookieToReplace, Set<BookieSocketAddress>
> > > > excludeBookies) throws BKNotEnoughBookiesException;
> > > >
> > > > public List<Integer>
> reorderReadSequence(List<BookieSocketAddress>
> > > > ensemble,
> > > > List<Integer> writeSet,
> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
> > > >
> > > >
> > > > public List<Integer> reorderReadLACSequence(List<
> > BookieSocketAddress>
> > > > ensemble,
> > > > List<Integer>
> writeSet,
> > > > Map<BookieSocketAddress, Long> bookieFailureHistory);
> > > >
> > > > default public void updateBookieInfo(Map<BookieSocketAddress,
> > > > BookieInfo> bookieInfoMap) {
> > > > }
> > > > }
> > > >
> > > >
> > > > where ClientEnvironment and LedgerSpecs contain all of the data which
> > > need
> > > > to be passed to the policy.
> > > > We can do even more better and group the other parameters
> > > >
> > > > we can evaluate to switch to an abstract class, or provide an
> official
> > > > "Adapter", but DefaultEnsemblePlacementPolicy is already a good
> "base"
> > > >
> > >
> > >
> > > wrapping fields into classes has same issues with adding fields to
> method
> > > signature. there isn't really difference when multiple version of jars
> > are
> > > in the classpath.
> > >
> >
> > It will let us add more contextual data without breaking the signatures.
> > Old code won't use new facilities and will work.
> > In general methods with manu parameters are not a good practice and many
> > overloaded version of the methid become easily tricky to maintain
> >
>
> Enrico, new code will be break if old jar is in the method. that's what I
> am trying to explain it to you - on the multiple version cases.
>
Sure. For 4.5 we have to way to work around and it is better ti break
compatibility. I only would like to prepare for the future and do the bes
> Both approaches have good and bad parts. So it is a preference and
> tradeoff.
>
Yup
>
>
> >
> >
> > > so it is really interesting for me to see why doesn't method
> overloading
> > > address the code binary backward compatibility issue.
> > >
> >
> > I will share some code soon
> >
> > Another blocker issue is that We should not use Guava in 'public' API
> > methods, as we are doing.
> > This will be easily resolved, we are usong Optional. I will send PR soon
> >
> > >
> > >
> > >
> > > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > I am ok with changing the API, I was the first to ask to change
> it
> > in
> > > > 4.5
> > > > > > in order to access custom metadata, but when we do such things I
> > > would
> > > > > like
> > > > > > to design a future proof API.
> > > > > >
> > > > > > I know it needs some time to design and I really to not want to
> > block
> > > > 4.5
> > > > > > release.
> > > > > > I will send a BP and maybe we can discuss on a concrete proposal.
> > > > > >
> > > > >
> > > > > Can we first make things clear here before any proposals?
> > > > >
> > > >
> > > > yep, sure.
> > > > I meant that writing all in a Wiki page makes it simpler for reviews
> > and
> > > > comments, in Kafka now they works this way: create KIP -> discussion
> ->
> > > > adjustments -> vote
> > > > this is another problem, to be discussed in another email thraed.
> > > >
> > >
> > > yes. but you already started an email thread to discuss this, no?
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > I think this change will break DL too.
> > > > > >
> > > > >
> > > > > When we upgraded the BK in DL, we will make some code changes to
> make
> > > > sure
> > > > > binary compatible.
> > > > >
> > > >
> > > >
> > > > I have referred to DL because it seems to me that it is explicit in
> DL
> > > > documentation that you can provide your own PlacementPolicy, so users
> > > which
> > > > implemented their own policy in DL will need to recompile.
> > > > In this specific case it it not really a problem because DL did not
> > work
> > > > with BK 4.4 at all, but it was just an example of a third party
> library
> > > > which depends on BK
> > > >
> > >
> > > I don't think we expose the placement policy directly in DL. We expose
> a
> > > DNS resolver instead.
> > > But it doesn't prevent user write his/her own placement policy. It
> might
> > > still have the problem
> > > you describe here.
> > >
> > >
> > >
> > >
> > > >
> > > >
> > > > >
> > > > >
> > > > > > Does this sound good to you?
> > > > > >
> > > > > > Enrico
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > As we are going to break the API now, would it be good to
> > create
> > > a
> > > > > more
> > > > > > > > future-proof API ?
> > > > > > >
> > > > > > > We can just post-pone the problem to 4.6
> > > > > > > >
> > > > > > > > Thoughts ?
> > > > > > > >
> > > > > > > > Enrico
> > > > > > > >
> > > > > > >
> > > > > > --
> > > > > >
> > > > > >
> > > > > > -- Enrico Olivelli
> > > > > >
> > > > >
> > > >
> > >
> > --
> >
> >
> > -- Enrico Olivelli
> >
>
--
-- Enrico Olivelli
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Sijie Guo <gu...@gmail.com>.
On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <eo...@gmail.com>
wrote:
> Il mar 25 lug 2017, 18:12 Sijie Guo <gu...@gmail.com> ha scritto:
>
> > Let me try to summarize the comments here (also as a reference for other
> > people). This basically is comprised of two questions:
> >
> > 1) how do we handle the 'binary backward compatibility' for drop-in
> upgrade
> > in applications using only one bk version?
> >
> > ideally if we change any method signatures in public API, it is a
> breaking
> > change. we should do this kind of change in a major version release.
> >
> > 4.5 is sort of a major version release because it attempts to merge
> changes
> > accumulated for years from multiple branches. Ideally we should bump the
> > release version to 5.0. but since it is already close to the release
> date,
> > I will prefer stick the version number to 4.5 but mark it as a major
> > release includes public interface changes (e.g. netty 4 upgrade, bytebuf
> > introduced, ensemble placement policy interface changed).
> >
> > method overloading should be able keep such binary backward compatibility
> > in general.
> >
> > 2) how do we handle the 'binary backward compatibility' for drop-in
> upgrade
> > in applications using multiple bk versions?
> >
> > This is tricky because it is really out of the scope of a single project.
> > It is typically a problem of JVM loading jars. Shading is more a solution
> > for 2).
> >
> >
> > If your problem is 1), method overloading should be able to take care of
> > it. You mentioned you tried method overloading, do you mind pointing me
> > your code change.
> > If your problem is 2), I do not see an easy way to address it even with
> > your proposal.
> >
> >
> > Other comments inline:
> >
> >
> > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <eo...@gmail.com>
> > wrote:
> >
> > > 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
> > >
> > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
> eolivelli@gmail.com>
> > > > wrote:
> > > >
> > > > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha
> scritto:
> > > > >
> > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > > eolivelli@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > > EnsemblePlacementPolicy interface.
> > > > > > >
> > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and
> > you
> > > > just
> > > > > > > switch the bookeeeper-server JAR on the classpath your
> > application
> > > > > won't
> > > > > > > run.
> > > > > > >
> > > > > > > I already got that problem and I needed to implement some
> > "support
> > > > 4.5"
> > > > > > > option in my projects which essentially tells "do not use a
> > custom
> > > > > > policy",
> > > > > > > this is very bad because there is no way to apply the custom
> > rules
> > > > > > required
> > > > > > > by the project.
> > > > > > >
> > > > > >
> > > > > > Do you mean the new methods introduced in placement policy? Or
> > > methods
> > > > > that
> > > > > > whose signatures are changed?
> > > > > >
> > > > >
> > > > > The new signatures
> > > > >
> > > > > >
> > > > > > I believe the new methods 'reorderSequence' is disabled by
> default
> > > > unless
> > > > > > you enable it explicitly.
> > > > > >
> > > > > > For methods that whose signatures were changed, we can add the
> old
> > > > > methods
> > > > > > back and create overrides to keep the binary backward compatible.
> > > > > >
> > > > >
> > > > > I don't know if this cam work. I have already tried. I will double
> > > check.
> > > > > Anyway it will be a bit messy
> > > > >
> > > > > >
> > > > > > However I am not sure if this works because the placement policy
> is
> > > > > > instantiated and loaded by reflection. Typically when you
> upgrade a
> > > > > > version, you have to compile it first, no?
> > > > > >
> > > > >
> > > > > I have several libraries which use bk and they are built on 4.4 and
> > > they
> > > > > are working together in the same classpath.
> > > > > For instance now I am going to change one of them in order to
> > leverage
> > > a
> > > > > new 4.5 feature, like readUnconfirmedEntries just as an example,
> so
> > I
> > > > need
> > > > > to switch to 4.5 on the classpath but the other projects won't run.
> > > > >
> > > >
> > > > Are you talking about mixing 4.4 and 4.5 together?
> > > >
> > >
> > > Yes, as for most widely used libraries (like ZooKeeper) it is common to
> > put
> > > a "new version" or the library on a application compiled for a previous
> > > version.
> > > I see that sometimes it is not possible, and for this reason we use
> > 'major
> > > versions' vs 'minor versions'.
> > > When possible I think it is best not to introduce such breaking
> changes.
> > >
> >
> > I agree with you. That's the purpose of 'major version' and 'minor
> > version'. However,
> > 4.5 is a special case which it was an attempt to merge multiple branches
> > together.
> > Although it doesn't break the protocol backward compatibility and code
> > compatibility
> > in public client API, it does change a bit on some interfaces (for
> example
> > the placement policy interface).
> >
>
> Yes, we did it for the AuthPlugin API too.
>
>
> >
> > >
> > >
> > >
> > >
> > >
> > > >
> > > >
> > > > > For the server side this is not a problem but on the client it is a
> > > real
> > > > > production problem.
> > > > >
> > > >
> > > > Can you clarify more specifically on this? Not very sure I understand
> > > your
> > > > problem and what are you going to achieve.
> > > >
> > > >
> > > Usually you have full control on bookies, but it is not always possible
> > to
> > > have full control of applications assembler from different components.
> > > Example:
> > > - I have Lib 1 v1 which is compiled with BK 4.4
> > > - I have Lib 2 v1 which is compiled with BK 4.4
> > > - I have App which depends on Lib1 + Lib2
> > > - Now Lib 2 v2 need BK 4.5 for a fresh new feature
> > > - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and will
> > > need to include BK 4.5, but this won't work
> > > Assemblers for App v2 do not have control on Lib1 and Lib2
> > >
> >
> > I am not sure if we should really deal with mixed version case. If you
> have
> > multiple version cases, you
> > might prefer shading the dependencies. Newer version of the library tends
> > to add new methods for new
> > features. If the class loader loads a class with old version, it will
> > anyway break your application using newer
> > library.
> >
> > Your question here is more about "How can we do better
> > to handle the use case of using multiple different versions of library in
> > same JVM".
> >
> >
> > >
> > > For instance I got into this trouble while integrating my new project
> > which
> > > uses readUnconfirmedEntries inside an App which is compiled for BK 4.4
> > and
> > > uses a custom placement policy.
> > >
> > > So, now that we are breaking things I would like to break them in a way
> > > that in the future will be simpler to handle
> > >
> > > for instance:
> > >
> > > public interface EnsemblePlacementPolicy {
> > >
> > >
> > > public EnsemblePlacementPolicy initialize(ClientEnvironment env);
> > >
> > > public void uninitalize();
> > >
> > > public Set<BookieSocketAddress>
> > > onClusterChanged(Set<BookieSocketAddress> writableBookies,
> > >
> > > Set<BookieSocketAddress> readOnlyBookies);
> > >
> > > public List<BookieSocketAddress> newEnsemble(LedgerSpecs
> ledgerSpecs,
> > > Set<BookieSocketAddress> excludeBookies) throws
> > > BKNotEnoughBookiesException;
> > >
> > >
> > > public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
> > > Collection<BookieSocketAddress> currentEnsemble,
> > > BookieSocketAddress bookieToReplace, Set<BookieSocketAddress>
> > > excludeBookies) throws BKNotEnoughBookiesException;
> > >
> > > public List<Integer> reorderReadSequence(List<BookieSocketAddress>
> > > ensemble,
> > > List<Integer> writeSet,
> > > Map<BookieSocketAddress, Long> bookieFailureHistory);
> > >
> > >
> > > public List<Integer> reorderReadLACSequence(List<
> BookieSocketAddress>
> > > ensemble,
> > > List<Integer> writeSet,
> > > Map<BookieSocketAddress, Long> bookieFailureHistory);
> > >
> > > default public void updateBookieInfo(Map<BookieSocketAddress,
> > > BookieInfo> bookieInfoMap) {
> > > }
> > > }
> > >
> > >
> > > where ClientEnvironment and LedgerSpecs contain all of the data which
> > need
> > > to be passed to the policy.
> > > We can do even more better and group the other parameters
> > >
> > > we can evaluate to switch to an abstract class, or provide an official
> > > "Adapter", but DefaultEnsemblePlacementPolicy is already a good "base"
> > >
> >
> >
> > wrapping fields into classes has same issues with adding fields to method
> > signature. there isn't really difference when multiple version of jars
> are
> > in the classpath.
> >
>
> It will let us add more contextual data without breaking the signatures.
> Old code won't use new facilities and will work.
> In general methods with manu parameters are not a good practice and many
> overloaded version of the methid become easily tricky to maintain
>
Enrico, new code will be break if old jar is in the method. that's what I
am trying to explain it to you - on the multiple version cases.
Both approaches have good and bad parts. So it is a preference and tradeoff.
>
>
> > so it is really interesting for me to see why doesn't method overloading
> > address the code binary backward compatibility issue.
> >
>
> I will share some code soon
>
> Another blocker issue is that We should not use Guava in 'public' API
> methods, as we are doing.
> This will be easily resolved, we are usong Optional. I will send PR soon
>
> >
> >
> >
> > >
> > >
> > >
> > > >
> > > >
> > > > > I am ok with changing the API, I was the first to ask to change it
> in
> > > 4.5
> > > > > in order to access custom metadata, but when we do such things I
> > would
> > > > like
> > > > > to design a future proof API.
> > > > >
> > > > > I know it needs some time to design and I really to not want to
> block
> > > 4.5
> > > > > release.
> > > > > I will send a BP and maybe we can discuss on a concrete proposal.
> > > > >
> > > >
> > > > Can we first make things clear here before any proposals?
> > > >
> > >
> > > yep, sure.
> > > I meant that writing all in a Wiki page makes it simpler for reviews
> and
> > > comments, in Kafka now they works this way: create KIP -> discussion ->
> > > adjustments -> vote
> > > this is another problem, to be discussed in another email thraed.
> > >
> >
> > yes. but you already started an email thread to discuss this, no?
> >
> >
> > >
> > > >
> > > >
> > > > > I think this change will break DL too.
> > > > >
> > > >
> > > > When we upgraded the BK in DL, we will make some code changes to make
> > > sure
> > > > binary compatible.
> > > >
> > >
> > >
> > > I have referred to DL because it seems to me that it is explicit in DL
> > > documentation that you can provide your own PlacementPolicy, so users
> > which
> > > implemented their own policy in DL will need to recompile.
> > > In this specific case it it not really a problem because DL did not
> work
> > > with BK 4.4 at all, but it was just an example of a third party library
> > > which depends on BK
> > >
> >
> > I don't think we expose the placement policy directly in DL. We expose a
> > DNS resolver instead.
> > But it doesn't prevent user write his/her own placement policy. It might
> > still have the problem
> > you describe here.
> >
> >
> >
> >
> > >
> > >
> > > >
> > > >
> > > > > Does this sound good to you?
> > > > >
> > > > > Enrico
> > > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > As we are going to break the API now, would it be good to
> create
> > a
> > > > more
> > > > > > > future-proof API ?
> > > > > >
> > > > > > We can just post-pone the problem to 4.6
> > > > > > >
> > > > > > > Thoughts ?
> > > > > > >
> > > > > > > Enrico
> > > > > > >
> > > > > >
> > > > > --
> > > > >
> > > > >
> > > > > -- Enrico Olivelli
> > > > >
> > > >
> > >
> >
> --
>
>
> -- Enrico Olivelli
>
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
Il mar 25 lug 2017, 18:12 Sijie Guo <gu...@gmail.com> ha scritto:
> Let me try to summarize the comments here (also as a reference for other
> people). This basically is comprised of two questions:
>
> 1) how do we handle the 'binary backward compatibility' for drop-in upgrade
> in applications using only one bk version?
>
> ideally if we change any method signatures in public API, it is a breaking
> change. we should do this kind of change in a major version release.
>
> 4.5 is sort of a major version release because it attempts to merge changes
> accumulated for years from multiple branches. Ideally we should bump the
> release version to 5.0. but since it is already close to the release date,
> I will prefer stick the version number to 4.5 but mark it as a major
> release includes public interface changes (e.g. netty 4 upgrade, bytebuf
> introduced, ensemble placement policy interface changed).
>
> method overloading should be able keep such binary backward compatibility
> in general.
>
> 2) how do we handle the 'binary backward compatibility' for drop-in upgrade
> in applications using multiple bk versions?
>
> This is tricky because it is really out of the scope of a single project.
> It is typically a problem of JVM loading jars. Shading is more a solution
> for 2).
>
>
> If your problem is 1), method overloading should be able to take care of
> it. You mentioned you tried method overloading, do you mind pointing me
> your code change.
> If your problem is 2), I do not see an easy way to address it even with
> your proposal.
>
>
> Other comments inline:
>
>
> On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <eo...@gmail.com>
> wrote:
>
> > 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
> >
> > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <eo...@gmail.com>
> > > wrote:
> > >
> > > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha scritto:
> > > >
> > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > eolivelli@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > EnsemblePlacementPolicy interface.
> > > > > >
> > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and
> you
> > > just
> > > > > > switch the bookeeeper-server JAR on the classpath your
> application
> > > > won't
> > > > > > run.
> > > > > >
> > > > > > I already got that problem and I needed to implement some
> "support
> > > 4.5"
> > > > > > option in my projects which essentially tells "do not use a
> custom
> > > > > policy",
> > > > > > this is very bad because there is no way to apply the custom
> rules
> > > > > required
> > > > > > by the project.
> > > > > >
> > > > >
> > > > > Do you mean the new methods introduced in placement policy? Or
> > methods
> > > > that
> > > > > whose signatures are changed?
> > > > >
> > > >
> > > > The new signatures
> > > >
> > > > >
> > > > > I believe the new methods 'reorderSequence' is disabled by default
> > > unless
> > > > > you enable it explicitly.
> > > > >
> > > > > For methods that whose signatures were changed, we can add the old
> > > > methods
> > > > > back and create overrides to keep the binary backward compatible.
> > > > >
> > > >
> > > > I don't know if this cam work. I have already tried. I will double
> > check.
> > > > Anyway it will be a bit messy
> > > >
> > > > >
> > > > > However I am not sure if this works because the placement policy is
> > > > > instantiated and loaded by reflection. Typically when you upgrade a
> > > > > version, you have to compile it first, no?
> > > > >
> > > >
> > > > I have several libraries which use bk and they are built on 4.4 and
> > they
> > > > are working together in the same classpath.
> > > > For instance now I am going to change one of them in order to
> leverage
> > a
> > > > new 4.5 feature, like readUnconfirmedEntries just as an example, so
> I
> > > need
> > > > to switch to 4.5 on the classpath but the other projects won't run.
> > > >
> > >
> > > Are you talking about mixing 4.4 and 4.5 together?
> > >
> >
> > Yes, as for most widely used libraries (like ZooKeeper) it is common to
> put
> > a "new version" or the library on a application compiled for a previous
> > version.
> > I see that sometimes it is not possible, and for this reason we use
> 'major
> > versions' vs 'minor versions'.
> > When possible I think it is best not to introduce such breaking changes.
> >
>
> I agree with you. That's the purpose of 'major version' and 'minor
> version'. However,
> 4.5 is a special case which it was an attempt to merge multiple branches
> together.
> Although it doesn't break the protocol backward compatibility and code
> compatibility
> in public client API, it does change a bit on some interfaces (for example
> the placement policy interface).
>
Yes, we did it for the AuthPlugin API too.
>
> >
> >
> >
> >
> >
> > >
> > >
> > > > For the server side this is not a problem but on the client it is a
> > real
> > > > production problem.
> > > >
> > >
> > > Can you clarify more specifically on this? Not very sure I understand
> > your
> > > problem and what are you going to achieve.
> > >
> > >
> > Usually you have full control on bookies, but it is not always possible
> to
> > have full control of applications assembler from different components.
> > Example:
> > - I have Lib 1 v1 which is compiled with BK 4.4
> > - I have Lib 2 v1 which is compiled with BK 4.4
> > - I have App which depends on Lib1 + Lib2
> > - Now Lib 2 v2 need BK 4.5 for a fresh new feature
> > - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and will
> > need to include BK 4.5, but this won't work
> > Assemblers for App v2 do not have control on Lib1 and Lib2
> >
>
> I am not sure if we should really deal with mixed version case. If you have
> multiple version cases, you
> might prefer shading the dependencies. Newer version of the library tends
> to add new methods for new
> features. If the class loader loads a class with old version, it will
> anyway break your application using newer
> library.
>
> Your question here is more about "How can we do better
> to handle the use case of using multiple different versions of library in
> same JVM".
>
>
> >
> > For instance I got into this trouble while integrating my new project
> which
> > uses readUnconfirmedEntries inside an App which is compiled for BK 4.4
> and
> > uses a custom placement policy.
> >
> > So, now that we are breaking things I would like to break them in a way
> > that in the future will be simpler to handle
> >
> > for instance:
> >
> > public interface EnsemblePlacementPolicy {
> >
> >
> > public EnsemblePlacementPolicy initialize(ClientEnvironment env);
> >
> > public void uninitalize();
> >
> > public Set<BookieSocketAddress>
> > onClusterChanged(Set<BookieSocketAddress> writableBookies,
> >
> > Set<BookieSocketAddress> readOnlyBookies);
> >
> > public List<BookieSocketAddress> newEnsemble(LedgerSpecs ledgerSpecs,
> > Set<BookieSocketAddress> excludeBookies) throws
> > BKNotEnoughBookiesException;
> >
> >
> > public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
> > Collection<BookieSocketAddress> currentEnsemble,
> > BookieSocketAddress bookieToReplace, Set<BookieSocketAddress>
> > excludeBookies) throws BKNotEnoughBookiesException;
> >
> > public List<Integer> reorderReadSequence(List<BookieSocketAddress>
> > ensemble,
> > List<Integer> writeSet,
> > Map<BookieSocketAddress, Long> bookieFailureHistory);
> >
> >
> > public List<Integer> reorderReadLACSequence(List<BookieSocketAddress>
> > ensemble,
> > List<Integer> writeSet,
> > Map<BookieSocketAddress, Long> bookieFailureHistory);
> >
> > default public void updateBookieInfo(Map<BookieSocketAddress,
> > BookieInfo> bookieInfoMap) {
> > }
> > }
> >
> >
> > where ClientEnvironment and LedgerSpecs contain all of the data which
> need
> > to be passed to the policy.
> > We can do even more better and group the other parameters
> >
> > we can evaluate to switch to an abstract class, or provide an official
> > "Adapter", but DefaultEnsemblePlacementPolicy is already a good "base"
> >
>
>
> wrapping fields into classes has same issues with adding fields to method
> signature. there isn't really difference when multiple version of jars are
> in the classpath.
>
It will let us add more contextual data without breaking the signatures.
Old code won't use new facilities and will work.
In general methods with manu parameters are not a good practice and many
overloaded version of the methid become easily tricky to maintain
> so it is really interesting for me to see why doesn't method overloading
> address the code binary backward compatibility issue.
>
I will share some code soon
Another blocker issue is that We should not use Guava in 'public' API
methods, as we are doing.
This will be easily resolved, we are usong Optional. I will send PR soon
>
>
>
> >
> >
> >
> > >
> > >
> > > > I am ok with changing the API, I was the first to ask to change it in
> > 4.5
> > > > in order to access custom metadata, but when we do such things I
> would
> > > like
> > > > to design a future proof API.
> > > >
> > > > I know it needs some time to design and I really to not want to block
> > 4.5
> > > > release.
> > > > I will send a BP and maybe we can discuss on a concrete proposal.
> > > >
> > >
> > > Can we first make things clear here before any proposals?
> > >
> >
> > yep, sure.
> > I meant that writing all in a Wiki page makes it simpler for reviews and
> > comments, in Kafka now they works this way: create KIP -> discussion ->
> > adjustments -> vote
> > this is another problem, to be discussed in another email thraed.
> >
>
> yes. but you already started an email thread to discuss this, no?
>
>
> >
> > >
> > >
> > > > I think this change will break DL too.
> > > >
> > >
> > > When we upgraded the BK in DL, we will make some code changes to make
> > sure
> > > binary compatible.
> > >
> >
> >
> > I have referred to DL because it seems to me that it is explicit in DL
> > documentation that you can provide your own PlacementPolicy, so users
> which
> > implemented their own policy in DL will need to recompile.
> > In this specific case it it not really a problem because DL did not work
> > with BK 4.4 at all, but it was just an example of a third party library
> > which depends on BK
> >
>
> I don't think we expose the placement policy directly in DL. We expose a
> DNS resolver instead.
> But it doesn't prevent user write his/her own placement policy. It might
> still have the problem
> you describe here.
>
>
>
>
> >
> >
> > >
> > >
> > > > Does this sound good to you?
> > > >
> > > > Enrico
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > As we are going to break the API now, would it be good to create
> a
> > > more
> > > > > > future-proof API ?
> > > > >
> > > > > We can just post-pone the problem to 4.6
> > > > > >
> > > > > > Thoughts ?
> > > > > >
> > > > > > Enrico
> > > > > >
> > > > >
> > > > --
> > > >
> > > >
> > > > -- Enrico Olivelli
> > > >
> > >
> >
>
--
-- Enrico Olivelli
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Sijie Guo <gu...@gmail.com>.
Let me try to summarize the comments here (also as a reference for other
people). This basically is comprised of two questions:
1) how do we handle the 'binary backward compatibility' for drop-in upgrade
in applications using only one bk version?
ideally if we change any method signatures in public API, it is a breaking
change. we should do this kind of change in a major version release.
4.5 is sort of a major version release because it attempts to merge changes
accumulated for years from multiple branches. Ideally we should bump the
release version to 5.0. but since it is already close to the release date,
I will prefer stick the version number to 4.5 but mark it as a major
release includes public interface changes (e.g. netty 4 upgrade, bytebuf
introduced, ensemble placement policy interface changed).
method overloading should be able keep such binary backward compatibility
in general.
2) how do we handle the 'binary backward compatibility' for drop-in upgrade
in applications using multiple bk versions?
This is tricky because it is really out of the scope of a single project.
It is typically a problem of JVM loading jars. Shading is more a solution
for 2).
If your problem is 1), method overloading should be able to take care of
it. You mentioned you tried method overloading, do you mind pointing me
your code change.
If your problem is 2), I do not see an easy way to address it even with
your proposal.
Other comments inline:
On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli <eo...@gmail.com>
wrote:
> 2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
>
> > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <eo...@gmail.com>
> > wrote:
> >
> > > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha scritto:
> > >
> > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> eolivelli@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi,
> > > > > we are going to release 4.5 soon, in this release we changed
> > > > > EnsemblePlacementPolicy interface.
> > > > >
> > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you
> > just
> > > > > switch the bookeeeper-server JAR on the classpath your application
> > > won't
> > > > > run.
> > > > >
> > > > > I already got that problem and I needed to implement some "support
> > 4.5"
> > > > > option in my projects which essentially tells "do not use a custom
> > > > policy",
> > > > > this is very bad because there is no way to apply the custom rules
> > > > required
> > > > > by the project.
> > > > >
> > > >
> > > > Do you mean the new methods introduced in placement policy? Or
> methods
> > > that
> > > > whose signatures are changed?
> > > >
> > >
> > > The new signatures
> > >
> > > >
> > > > I believe the new methods 'reorderSequence' is disabled by default
> > unless
> > > > you enable it explicitly.
> > > >
> > > > For methods that whose signatures were changed, we can add the old
> > > methods
> > > > back and create overrides to keep the binary backward compatible.
> > > >
> > >
> > > I don't know if this cam work. I have already tried. I will double
> check.
> > > Anyway it will be a bit messy
> > >
> > > >
> > > > However I am not sure if this works because the placement policy is
> > > > instantiated and loaded by reflection. Typically when you upgrade a
> > > > version, you have to compile it first, no?
> > > >
> > >
> > > I have several libraries which use bk and they are built on 4.4 and
> they
> > > are working together in the same classpath.
> > > For instance now I am going to change one of them in order to leverage
> a
> > > new 4.5 feature, like readUnconfirmedEntries just as an example, so I
> > need
> > > to switch to 4.5 on the classpath but the other projects won't run.
> > >
> >
> > Are you talking about mixing 4.4 and 4.5 together?
> >
>
> Yes, as for most widely used libraries (like ZooKeeper) it is common to put
> a "new version" or the library on a application compiled for a previous
> version.
> I see that sometimes it is not possible, and for this reason we use 'major
> versions' vs 'minor versions'.
> When possible I think it is best not to introduce such breaking changes.
>
I agree with you. That's the purpose of 'major version' and 'minor
version'. However,
4.5 is a special case which it was an attempt to merge multiple branches
together.
Although it doesn't break the protocol backward compatibility and code
compatibility
in public client API, it does change a bit on some interfaces (for example
the placement policy interface).
>
>
>
>
>
> >
> >
> > > For the server side this is not a problem but on the client it is a
> real
> > > production problem.
> > >
> >
> > Can you clarify more specifically on this? Not very sure I understand
> your
> > problem and what are you going to achieve.
> >
> >
> Usually you have full control on bookies, but it is not always possible to
> have full control of applications assembler from different components.
> Example:
> - I have Lib 1 v1 which is compiled with BK 4.4
> - I have Lib 2 v1 which is compiled with BK 4.4
> - I have App which depends on Lib1 + Lib2
> - Now Lib 2 v2 need BK 4.5 for a fresh new feature
> - New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and will
> need to include BK 4.5, but this won't work
> Assemblers for App v2 do not have control on Lib1 and Lib2
>
I am not sure if we should really deal with mixed version case. If you have
multiple version cases, you
might prefer shading the dependencies. Newer version of the library tends
to add new methods for new
features. If the class loader loads a class with old version, it will
anyway break your application using newer
library.
Your question here is more about "How can we do better
to handle the use case of using multiple different versions of library in
same JVM".
>
> For instance I got into this trouble while integrating my new project which
> uses readUnconfirmedEntries inside an App which is compiled for BK 4.4 and
> uses a custom placement policy.
>
> So, now that we are breaking things I would like to break them in a way
> that in the future will be simpler to handle
>
> for instance:
>
> public interface EnsemblePlacementPolicy {
>
>
> public EnsemblePlacementPolicy initialize(ClientEnvironment env);
>
> public void uninitalize();
>
> public Set<BookieSocketAddress>
> onClusterChanged(Set<BookieSocketAddress> writableBookies,
>
> Set<BookieSocketAddress> readOnlyBookies);
>
> public List<BookieSocketAddress> newEnsemble(LedgerSpecs ledgerSpecs,
> Set<BookieSocketAddress> excludeBookies) throws
> BKNotEnoughBookiesException;
>
>
> public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
> Collection<BookieSocketAddress> currentEnsemble,
> BookieSocketAddress bookieToReplace, Set<BookieSocketAddress>
> excludeBookies) throws BKNotEnoughBookiesException;
>
> public List<Integer> reorderReadSequence(List<BookieSocketAddress>
> ensemble,
> List<Integer> writeSet,
> Map<BookieSocketAddress, Long> bookieFailureHistory);
>
>
> public List<Integer> reorderReadLACSequence(List<BookieSocketAddress>
> ensemble,
> List<Integer> writeSet,
> Map<BookieSocketAddress, Long> bookieFailureHistory);
>
> default public void updateBookieInfo(Map<BookieSocketAddress,
> BookieInfo> bookieInfoMap) {
> }
> }
>
>
> where ClientEnvironment and LedgerSpecs contain all of the data which need
> to be passed to the policy.
> We can do even more better and group the other parameters
>
> we can evaluate to switch to an abstract class, or provide an official
> "Adapter", but DefaultEnsemblePlacementPolicy is already a good "base"
>
wrapping fields into classes has same issues with adding fields to method
signature. there isn't really difference when multiple version of jars are
in the classpath.
so it is really interesting for me to see why doesn't method overloading
address the code binary backward compatibility issue.
>
>
>
> >
> >
> > > I am ok with changing the API, I was the first to ask to change it in
> 4.5
> > > in order to access custom metadata, but when we do such things I would
> > like
> > > to design a future proof API.
> > >
> > > I know it needs some time to design and I really to not want to block
> 4.5
> > > release.
> > > I will send a BP and maybe we can discuss on a concrete proposal.
> > >
> >
> > Can we first make things clear here before any proposals?
> >
>
> yep, sure.
> I meant that writing all in a Wiki page makes it simpler for reviews and
> comments, in Kafka now they works this way: create KIP -> discussion ->
> adjustments -> vote
> this is another problem, to be discussed in another email thraed.
>
yes. but you already started an email thread to discuss this, no?
>
> >
> >
> > > I think this change will break DL too.
> > >
> >
> > When we upgraded the BK in DL, we will make some code changes to make
> sure
> > binary compatible.
> >
>
>
> I have referred to DL because it seems to me that it is explicit in DL
> documentation that you can provide your own PlacementPolicy, so users which
> implemented their own policy in DL will need to recompile.
> In this specific case it it not really a problem because DL did not work
> with BK 4.4 at all, but it was just an example of a third party library
> which depends on BK
>
I don't think we expose the placement policy directly in DL. We expose a
DNS resolver instead.
But it doesn't prevent user write his/her own placement policy. It might
still have the problem
you describe here.
>
>
> >
> >
> > > Does this sound good to you?
> > >
> > > Enrico
> > >
> > >
> > > >
> > > > >
> > > > > As we are going to break the API now, would it be good to create a
> > more
> > > > > future-proof API ?
> > > >
> > > > We can just post-pone the problem to 4.6
> > > > >
> > > > > Thoughts ?
> > > > >
> > > > > Enrico
> > > > >
> > > >
> > > --
> > >
> > >
> > > -- Enrico Olivelli
> > >
> >
>
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
2017-07-24 22:54 GMT+02:00 Sijie Guo <gu...@gmail.com>:
> On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <eo...@gmail.com>
> wrote:
>
> > Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha scritto:
> >
> > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <eo...@gmail.com>
> > > wrote:
> > >
> > > > Hi,
> > > > we are going to release 4.5 soon, in this release we changed
> > > > EnsemblePlacementPolicy interface.
> > > >
> > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you
> just
> > > > switch the bookeeeper-server JAR on the classpath your application
> > won't
> > > > run.
> > > >
> > > > I already got that problem and I needed to implement some "support
> 4.5"
> > > > option in my projects which essentially tells "do not use a custom
> > > policy",
> > > > this is very bad because there is no way to apply the custom rules
> > > required
> > > > by the project.
> > > >
> > >
> > > Do you mean the new methods introduced in placement policy? Or methods
> > that
> > > whose signatures are changed?
> > >
> >
> > The new signatures
> >
> > >
> > > I believe the new methods 'reorderSequence' is disabled by default
> unless
> > > you enable it explicitly.
> > >
> > > For methods that whose signatures were changed, we can add the old
> > methods
> > > back and create overrides to keep the binary backward compatible.
> > >
> >
> > I don't know if this cam work. I have already tried. I will double check.
> > Anyway it will be a bit messy
> >
> > >
> > > However I am not sure if this works because the placement policy is
> > > instantiated and loaded by reflection. Typically when you upgrade a
> > > version, you have to compile it first, no?
> > >
> >
> > I have several libraries which use bk and they are built on 4.4 and they
> > are working together in the same classpath.
> > For instance now I am going to change one of them in order to leverage a
> > new 4.5 feature, like readUnconfirmedEntries just as an example, so I
> need
> > to switch to 4.5 on the classpath but the other projects won't run.
> >
>
> Are you talking about mixing 4.4 and 4.5 together?
>
Yes, as for most widely used libraries (like ZooKeeper) it is common to put
a "new version" or the library on a application compiled for a previous
version.
I see that sometimes it is not possible, and for this reason we use 'major
versions' vs 'minor versions'.
When possible I think it is best not to introduce such breaking changes.
>
>
> > For the server side this is not a problem but on the client it is a real
> > production problem.
> >
>
> Can you clarify more specifically on this? Not very sure I understand your
> problem and what are you going to achieve.
>
>
Usually you have full control on bookies, but it is not always possible to
have full control of applications assembler from different components.
Example:
- I have Lib 1 v1 which is compiled with BK 4.4
- I have Lib 2 v1 which is compiled with BK 4.4
- I have App which depends on Lib1 + Lib2
- Now Lib 2 v2 need BK 4.5 for a fresh new feature
- New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and will
need to include BK 4.5, but this won't work
Assemblers for App v2 do not have control on Lib1 and Lib2
For instance I got into this trouble while integrating my new project which
uses readUnconfirmedEntries inside an App which is compiled for BK 4.4 and
uses a custom placement policy.
So, now that we are breaking things I would like to break them in a way
that in the future will be simpler to handle
for instance:
public interface EnsemblePlacementPolicy {
public EnsemblePlacementPolicy initialize(ClientEnvironment env);
public void uninitalize();
public Set<BookieSocketAddress>
onClusterChanged(Set<BookieSocketAddress> writableBookies,
Set<BookieSocketAddress> readOnlyBookies);
public List<BookieSocketAddress> newEnsemble(LedgerSpecs ledgerSpecs,
Set<BookieSocketAddress> excludeBookies) throws BKNotEnoughBookiesException;
public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
Collection<BookieSocketAddress> currentEnsemble,
BookieSocketAddress bookieToReplace, Set<BookieSocketAddress>
excludeBookies) throws BKNotEnoughBookiesException;
public List<Integer> reorderReadSequence(List<BookieSocketAddress>
ensemble,
List<Integer> writeSet,
Map<BookieSocketAddress, Long> bookieFailureHistory);
public List<Integer> reorderReadLACSequence(List<BookieSocketAddress>
ensemble,
List<Integer> writeSet,
Map<BookieSocketAddress, Long> bookieFailureHistory);
default public void updateBookieInfo(Map<BookieSocketAddress,
BookieInfo> bookieInfoMap) {
}
}
where ClientEnvironment and LedgerSpecs contain all of the data which need
to be passed to the policy.
We can do even more better and group the other parameters
we can evaluate to switch to an abstract class, or provide an official
"Adapter", but DefaultEnsemblePlacementPolicy is already a good "base"
>
>
> > I am ok with changing the API, I was the first to ask to change it in 4.5
> > in order to access custom metadata, but when we do such things I would
> like
> > to design a future proof API.
> >
> > I know it needs some time to design and I really to not want to block 4.5
> > release.
> > I will send a BP and maybe we can discuss on a concrete proposal.
> >
>
> Can we first make things clear here before any proposals?
>
yep, sure.
I meant that writing all in a Wiki page makes it simpler for reviews and
comments, in Kafka now they works this way: create KIP -> discussion ->
adjustments -> vote
this is another problem, to be discussed in another email thraed.
>
>
> > I think this change will break DL too.
> >
>
> When we upgraded the BK in DL, we will make some code changes to make sure
> binary compatible.
>
I have referred to DL because it seems to me that it is explicit in DL
documentation that you can provide your own PlacementPolicy, so users which
implemented their own policy in DL will need to recompile.
In this specific case it it not really a problem because DL did not work
with BK 4.4 at all, but it was just an example of a third party library
which depends on BK
>
>
> > Does this sound good to you?
> >
> > Enrico
> >
> >
> > >
> > > >
> > > > As we are going to break the API now, would it be good to create a
> more
> > > > future-proof API ?
> > >
> > > We can just post-pone the problem to 4.6
> > > >
> > > > Thoughts ?
> > > >
> > > > Enrico
> > > >
> > >
> > --
> >
> >
> > -- Enrico Olivelli
> >
>
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Sijie Guo <gu...@gmail.com>.
On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <eo...@gmail.com>
wrote:
> Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha scritto:
>
> > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <eo...@gmail.com>
> > wrote:
> >
> > > Hi,
> > > we are going to release 4.5 soon, in this release we changed
> > > EnsemblePlacementPolicy interface.
> > >
> > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you just
> > > switch the bookeeeper-server JAR on the classpath your application
> won't
> > > run.
> > >
> > > I already got that problem and I needed to implement some "support 4.5"
> > > option in my projects which essentially tells "do not use a custom
> > policy",
> > > this is very bad because there is no way to apply the custom rules
> > required
> > > by the project.
> > >
> >
> > Do you mean the new methods introduced in placement policy? Or methods
> that
> > whose signatures are changed?
> >
>
> The new signatures
>
> >
> > I believe the new methods 'reorderSequence' is disabled by default unless
> > you enable it explicitly.
> >
> > For methods that whose signatures were changed, we can add the old
> methods
> > back and create overrides to keep the binary backward compatible.
> >
>
> I don't know if this cam work. I have already tried. I will double check.
> Anyway it will be a bit messy
>
> >
> > However I am not sure if this works because the placement policy is
> > instantiated and loaded by reflection. Typically when you upgrade a
> > version, you have to compile it first, no?
> >
>
> I have several libraries which use bk and they are built on 4.4 and they
> are working together in the same classpath.
> For instance now I am going to change one of them in order to leverage a
> new 4.5 feature, like readUnconfirmedEntries just as an example, so I need
> to switch to 4.5 on the classpath but the other projects won't run.
>
Are you talking about mixing 4.4 and 4.5 together?
> For the server side this is not a problem but on the client it is a real
> production problem.
>
Can you clarify more specifically on this? Not very sure I understand your
problem and what are you going to achieve.
> I am ok with changing the API, I was the first to ask to change it in 4.5
> in order to access custom metadata, but when we do such things I would like
> to design a future proof API.
>
> I know it needs some time to design and I really to not want to block 4.5
> release.
> I will send a BP and maybe we can discuss on a concrete proposal.
>
Can we first make things clear here before any proposals?
> I think this change will break DL too.
>
When we upgraded the BK in DL, we will make some code changes to make sure
binary compatible.
> Does this sound good to you?
>
> Enrico
>
>
> >
> > >
> > > As we are going to break the API now, would it be good to create a more
> > > future-proof API ?
> >
> > We can just post-pone the problem to 4.6
> > >
> > > Thoughts ?
> > >
> > > Enrico
> > >
> >
> --
>
>
> -- Enrico Olivelli
>
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Enrico Olivelli <eo...@gmail.com>.
Il lun 24 lug 2017, 19:39 Sijie Guo <gu...@gmail.com> ha scritto:
> On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <eo...@gmail.com>
> wrote:
>
> > Hi,
> > we are going to release 4.5 soon, in this release we changed
> > EnsemblePlacementPolicy interface.
> >
> > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you just
> > switch the bookeeeper-server JAR on the classpath your application won't
> > run.
> >
> > I already got that problem and I needed to implement some "support 4.5"
> > option in my projects which essentially tells "do not use a custom
> policy",
> > this is very bad because there is no way to apply the custom rules
> required
> > by the project.
> >
>
> Do you mean the new methods introduced in placement policy? Or methods that
> whose signatures are changed?
>
The new signatures
>
> I believe the new methods 'reorderSequence' is disabled by default unless
> you enable it explicitly.
>
> For methods that whose signatures were changed, we can add the old methods
> back and create overrides to keep the binary backward compatible.
>
I don't know if this cam work. I have already tried. I will double check.
Anyway it will be a bit messy
>
> However I am not sure if this works because the placement policy is
> instantiated and loaded by reflection. Typically when you upgrade a
> version, you have to compile it first, no?
>
I have several libraries which use bk and they are built on 4.4 and they
are working together in the same classpath.
For instance now I am going to change one of them in order to leverage a
new 4.5 feature, like readUnconfirmedEntries just as an example, so I need
to switch to 4.5 on the classpath but the other projects won't run.
For the server side this is not a problem but on the client it is a real
production problem.
I am ok with changing the API, I was the first to ask to change it in 4.5
in order to access custom metadata, but when we do such things I would like
to design a future proof API.
I know it needs some time to design and I really to not want to block 4.5
release.
I will send a BP and maybe we can discuss on a concrete proposal.
I think this change will break DL too.
Does this sound good to you?
Enrico
>
> >
> > As we are going to break the API now, would it be good to create a more
> > future-proof API ?
>
> We can just post-pone the problem to 4.6
> >
> > Thoughts ?
> >
> > Enrico
> >
>
--
-- Enrico Olivelli
Re: EnsemblePlacementPolicy Public API breaking changes
Posted by Sijie Guo <gu...@gmail.com>.
On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <eo...@gmail.com>
wrote:
> Hi,
> we are going to release 4.5 soon, in this release we changed
> EnsemblePlacementPolicy interface.
>
> It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you just
> switch the bookeeeper-server JAR on the classpath your application won't
> run.
>
> I already got that problem and I needed to implement some "support 4.5"
> option in my projects which essentially tells "do not use a custom policy",
> this is very bad because there is no way to apply the custom rules required
> by the project.
>
Do you mean the new methods introduced in placement policy? Or methods that
whose signatures are changed?
I believe the new methods 'reorderSequence' is disabled by default unless
you enable it explicitly.
For methods that whose signatures were changed, we can add the old methods
back and create overrides to keep the binary backward compatible.
However I am not sure if this works because the placement policy is
instantiated and loaded by reflection. Typically when you upgrade a
version, you have to compile it first, no?
>
> As we are going to break the API now, would it be good to create a more
> future-proof API ?
We can just post-pone the problem to 4.6
>
> Thoughts ?
>
> Enrico
>