It will be good to see others' opinons before you updating the proposal.
- Sijie
On Fri, Sep 1, 2017 at 2:58 AM, Sijie Guo <guosi...@gmail.com> wrote:
>
>
> 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
>> > >
>> >
>>
>
>
