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 >