On Fri, Sep 1, 2017 at 2:50 AM, Enrico Olivelli <eolive...@gmail.com> wrote:
> 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
>
If we are using builder, then we don't this any more, no?
>
>
> > 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
>
one consideration should be taken is the object allocation. I am fine with
delegation.
but we should be looking into using netty recycler for reusing objects. I
believe Matteo did
some changes in yahoo branch (which hasn't been ported back). We should
consider
following that style.
>
>
> >
> > 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
> > >
> >
>
