On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <eolive...@gmail.com>
wrote:
> Il mar 25 lug 2017, 18:12 Sijie Guo <guosi...@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 <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).
> >
>
> 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
>
