2017-09-01 12:21 GMT+02:00 Sijie Guo <guosi...@gmail.com>:
> On Fri, Sep 1, 2017 at 3:04 AM, Enrico Olivelli <eolive...@gmail.com>
> wrote:
>
> > 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 ?
> >
>
> we are at netty 4 now. I would suggest if it is not difficult, we should
> incorporate that style.
> otherwise, there is no one to make such changes in future for this object.
>
OK I will try
>
>
> >
> >
> > 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 ?
> >
>
> In general, you should provide as much details as possible, especially for
> API interface changes.
>
got it
>
>
> > My feeling is that I can attach a PR as soon as we have consensus on
> > general approach
> >
>
> You are welcome to send a prototype pull request. But I would suggest
> holding on any code change because this is a public API change - we should
> get consensus in the community.
>
Yes, I will start after getting enough feedback
-- Enrico
>
>
> >
> > 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
> > > > > >
> > > > >
> > > >
> > >
> >
>
