Thank you Sijie for taking a look so quickly My proposal was only narrowed to the CreateLedger API, but I see your approach is really more straightforward and I like it very much. I have introduced the LedgerConfiguration bean in order to have some "template" for creating ledgers, you proposal is more complete.
I will surely update the document with your proposal, maybe we can give time for other comments some answers inline Enrico 2017-09-01 11:20 GMT+02:00 Sijie Guo <guosi...@gmail.com>: > hmm, I actually like the alternative you rejected. Instead, I don't think > the approach in the proposal is the right one that we should take. > > 1) LedgerConfiguration is a confusing term used here. what doest a > `configuration` mean here, is the configuration stored somewhere or just > purely runtime configuration? > It was for runtime, maybe a better name would be LedgerCreationSpecs > 2) It is not an extendible solution. how does this apply to open/delete > operations? > yes I agree, your proposal sounds really more powerful to me > > I'd like to extend your alternative to understand why it is rejected by > you. > > ===== The builder approach ========== > > 1) builder interfaces for operations > > - create operations builder interfaces under > org/apache/bookkeeper/client/api/op/ > > - add a ICreateBuilder for create operation: > > interface ICreateBuilder { > > ICreateBuilder withEnsembleSize(...); > > ICreateBuilder withWriteQuorumSize(...); > > ... > > void execute(CreateCallback callback, Object ctx); > > // support java8 completable future > CompletableFuture<LedgerHandle> execute(); > > } > > - LedgerCreateOp implements CreateBuilder > I don't like this very much, maybe I would like to create wrappers/delegates all of the XXXOps are quite complex, and this will make them even more complex. Anyway I see it would be simpler from another point of view, and it can be fine to be > > 2) create an interface for bookkeeper. IBookKeeper under > org/apache/bookkeeper/client/api/IBookKeeper > > interface IBookKeeper { > > /** > * new a create ledger builder. > */ > ICreateBuilder createLedger(); > > } > > class BookKeeper extends IBookKeeper { > > ICreateBuilder createLedger() { > > // return a ledger create op builder > > } > > } > > > there are a few benefits using this approach: > > - this approach can be used for other operations - create/delete/open. we > will have a consistent style for different ops and easy to extend in > future. > yes > - we can use this approach to cleanup all the interfaces. so we can > separate interface from implementation eventually, without breaking > existing API. then we can encourage people to use new API and phase out the > old API. > yes > - we should also have interface for LedgerHandle, try to separate write > interface from read interface to produce a cleaner interface. > I totally agree on this point ! LedgerHandle is somehow "messy", it contains both read and write operations, and the meaning of some variables (and so getters...) is "contextual" Having different interfaces will let use be clearer on the contracts with the client > > > Any thoughts? > > - Sijie > > > On Fri, Sep 1, 2017 at 1:33 AM, Enrico Olivelli <eolive...@gmail.com> > wrote: > > > Hi all, > > I have just posted a proposal to introduce a new createLedger API using > the > > 'builder' design pattern. > > > > https://cwiki.apache.org/confluence/display/BOOKKEEPER/ > > BP-15+New+CreateLeader+API > > > > This is a pre-requisite for BP-14 Relax Durability and for LedgerType > > improvements > > > > It is a trivial change but it will be the base for future enhancements > > > > I will send a patch as soon as we agree on the proposal > > > > Cheers > > Enrico Olivelli > > >