Il mar 25 lug 2017, 18:12 Sijie Guo <[email protected]> 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 <[email protected]>
> wrote:
>
> > 2017-07-24 22:54 GMT+02:00 Sijie Guo <[email protected]>:
> >
> > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <[email protected]>
> > > wrote:
> > >
> > > > Il lun 24 lug 2017, 19:39 Sijie Guo <[email protected]> ha scritto:
> > > >
> > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > [email protected]>
> > > > > 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
