This is the PR I have tried to address the issue with method overloading and interface default methods. https://github.com/apache/bookkeeper/pull/301
For my projects it is working We can defer to 4.6 eventually a complete refactor of this interface -- Enrico 2017-07-25 19:29 GMT+02:00 Enrico Olivelli <[email protected]>: > > > Il mar 25 lug 2017, 19:08 Sijie Guo <[email protected]> ha scritto: > >> On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli <[email protected]> >> wrote: >> >> > 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 >> > >> >> 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 >
