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.
>
>
> 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.
> 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.
>
> 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
> > > > >
> > > >
> > >
> >
>
