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 <eolive...@gmail.com>
wrote:
> 2017-07-24 22:54 GMT+02:00 Sijie Guo <guosi...@gmail.com>:
>
> > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <eolive...@gmail.com>
> > wrote:
> >
> > > Il lun 24 lug 2017, 19:39 Sijie Guo <guosi...@gmail.com> ha scritto:
> > >
> > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> eolive...@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
> > >
> >
>
