Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
Il giorno mer 23 gen 2019 alle ore 11:48 Ivan Kelly
ha scritto:
>
> > > ```
> > > class PlacementResult {
> > > T result();
> > > boolean strictlyConformsToPolicy();
> > > }
> >
> > That was my first proposal and I like it. It is clearer and
> > auto-documenting.
> >
> > Given that we are changing EnsemblePlacementPolicy at every major
> > version, we can defer this refactor to 4.10.
> > I am open to adopt this solution as well.
>
> It's a small change, so better to apply it now. If you don't have the
> cycles today, I can pick it up. We should merge your Pair stuff
> anyhow. Pair is always useful.
Ok, please pick up my patch, today I am very busy.
Thank you so much.
See you on slack as well
Enrico
>
> -Ivan
Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
> > ```
> > class PlacementResult {
> > T result();
> > boolean strictlyConformsToPolicy();
> > }
>
> That was my first proposal and I like it. It is clearer and auto-documenting.
>
> Given that we are changing EnsemblePlacementPolicy at every major
> version, we can defer this refactor to 4.10.
> I am open to adopt this solution as well.
It's a small change, so better to apply it now. If you don't have the
cycles today, I can pick it up. We should merge your Pair stuff
anyhow. Pair is always useful.
-Ivan
Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
Il giorno mer 23 gen 2019 alle ore 11:33 Ivan Kelly
ha scritto:
>
> There's no harm in having our own tuple implementation in common, but
> in this instance we should encode more meaning into the returned
> value. As it is, it's not even java documented.
> But in both cases, it looks like the boolean is whether the placement
> strictly conforms to the placement policy, so both could return.
>
> ```
> class PlacementResult {
> T result();
> boolean strictlyConformsToPolicy();
> }
That was my first proposal and I like it. It is clearer and auto-documenting.
Given that we are changing EnsemblePlacementPolicy at every major
version, we can defer this refactor to 4.10.
I am open to adopt this solution as well.
I have already prepared the PR for the new 'Pair' but it will be
really easy to change again.
https://github.com/apache/bookkeeper/pull/1915
Enrico
> ```
>
> -Ivan
>
> On Tue, Jan 22, 2019 at 6:55 PM Enrico Olivelli wrote:
> >
> > Il mar 22 gen 2019, 18:38 Sijie Guo ha scritto:
> >
> > > On Tue, Jan 22, 2019 at 8:40 AM Enrico Olivelli
> > > wrote:
> > >
> > > > Hi all,
> > > > while reviewing 4.9 release I found this problem around a change about
> > > > EnsemblePlacementPolicy
> > > >
> > > > this is the issue
> > > > https://github.com/apache/bookkeeper/issues/1914
> > > >
> > > > The problem is that in public API we should not use third party
> > > > classes in order to preserve compatibility with incompatible changes
> > > > of the third party library.
> > > >
> > > > We already had such problems in the past.
> > > >
> > > > I think the best way to address this problem is to introduce one
> > > > specific class in BookKeeper, maybe an inner class of
> > > > EnsemblePlacementPolicy.
> > > >
> > >
> > > why not just introduce a Pair like class in bookkeeper-common module?
> > > instead of an inner class for EnsemblePlacementPolicy.
> > >
> >
> > Works for me
> >
> > Enrico
> >
> > >
> > >
> > > >
> > > > If we agree on this solution I can send the patch, it is very
> > > > straightforward
> > > >
> > > > Regards
> > > > Enrico
> > > >
> > >
> > --
> >
> >
> > -- Enrico Olivelli
Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
There's no harm in having our own tuple implementation in common, but
in this instance we should encode more meaning into the returned
value. As it is, it's not even java documented.
But in both cases, it looks like the boolean is whether the placement
strictly conforms to the placement policy, so both could return.
```
class PlacementResult {
T result();
boolean strictlyConformsToPolicy();
}
```
-Ivan
On Tue, Jan 22, 2019 at 6:55 PM Enrico Olivelli wrote:
>
> Il mar 22 gen 2019, 18:38 Sijie Guo ha scritto:
>
> > On Tue, Jan 22, 2019 at 8:40 AM Enrico Olivelli
> > wrote:
> >
> > > Hi all,
> > > while reviewing 4.9 release I found this problem around a change about
> > > EnsemblePlacementPolicy
> > >
> > > this is the issue
> > > https://github.com/apache/bookkeeper/issues/1914
> > >
> > > The problem is that in public API we should not use third party
> > > classes in order to preserve compatibility with incompatible changes
> > > of the third party library.
> > >
> > > We already had such problems in the past.
> > >
> > > I think the best way to address this problem is to introduce one
> > > specific class in BookKeeper, maybe an inner class of
> > > EnsemblePlacementPolicy.
> > >
> >
> > why not just introduce a Pair like class in bookkeeper-common module?
> > instead of an inner class for EnsemblePlacementPolicy.
> >
>
> Works for me
>
> Enrico
>
> >
> >
> > >
> > > If we agree on this solution I can send the patch, it is very
> > > straightforward
> > >
> > > Regards
> > > Enrico
> > >
> >
> --
>
>
> -- Enrico Olivelli
Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
Il mar 22 gen 2019, 18:38 Sijie Guo ha scritto: > On Tue, Jan 22, 2019 at 8:40 AM Enrico Olivelli > wrote: > > > Hi all, > > while reviewing 4.9 release I found this problem around a change about > > EnsemblePlacementPolicy > > > > this is the issue > > https://github.com/apache/bookkeeper/issues/1914 > > > > The problem is that in public API we should not use third party > > classes in order to preserve compatibility with incompatible changes > > of the third party library. > > > > We already had such problems in the past. > > > > I think the best way to address this problem is to introduce one > > specific class in BookKeeper, maybe an inner class of > > EnsemblePlacementPolicy. > > > > why not just introduce a Pair like class in bookkeeper-common module? > instead of an inner class for EnsemblePlacementPolicy. > Works for me Enrico > > > > > > If we agree on this solution I can send the patch, it is very > > straightforward > > > > Regards > > Enrico > > > -- -- Enrico Olivelli
Re: EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
On Tue, Jan 22, 2019 at 8:40 AM Enrico Olivelli wrote: > Hi all, > while reviewing 4.9 release I found this problem around a change about > EnsemblePlacementPolicy > > this is the issue > https://github.com/apache/bookkeeper/issues/1914 > > The problem is that in public API we should not use third party > classes in order to preserve compatibility with incompatible changes > of the third party library. > > We already had such problems in the past. > > I think the best way to address this problem is to introduce one > specific class in BookKeeper, maybe an inner class of > EnsemblePlacementPolicy. > why not just introduce a Pair like class in bookkeeper-common module? instead of an inner class for EnsemblePlacementPolicy. > > If we agree on this solution I can send the patch, it is very > straightforward > > Regards > Enrico >
EnsemblePlacementPolicy exposes third party API "Pair" from commons-lang3 in a public API
Hi all, while reviewing 4.9 release I found this problem around a change about EnsemblePlacementPolicy this is the issue https://github.com/apache/bookkeeper/issues/1914 The problem is that in public API we should not use third party classes in order to preserve compatibility with incompatible changes of the third party library. We already had such problems in the past. I think the best way to address this problem is to introduce one specific class in BookKeeper, maybe an inner class of EnsemblePlacementPolicy. If we agree on this solution I can send the patch, it is very straightforward Regards Enrico
