Il mar 25 lug 2017, 19:08 Sijie Guo <guosi...@gmail.com> ha scritto:
> 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.
>
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
