2017-09-01 11:58 GMT+02:00 Sijie Guo <guosi...@gmail.com>:
> 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?
>
yes, I will drop it
>
> >
> >
> > > 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.
>
I saw that changes on Yahoo branch, we can start with allocating new
instances, and then introduce the recycler for this kind of object and the
other parts in Yahoo branch.
Does this sounds good to you ?
Side question:
Do you think that in the wiki page a brief description as your are writing
in this email thread would be enough ? O I should provide detailed changes ?
My feeling is that I can attach a PR as soon as we have consensus on
general approach
I will wait other opinions before updating the wiki page
Enrico
>
> >
> >
> > >
> > > 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
> > > >
> > >
> >
>
