The end of the story is that discussing with Sijie we decided to close the issue as won't fix. In the future we will define the rules to handle backward compatibility. Changes on the PlacementPolicy are about semantics from 4.4 to 4.5
Enrico Il mer 26 lug 2017, 12:26 Enrico Olivelli <[email protected]> ha scritto: > 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 >> > > -- -- Enrico Olivelli
