Re: Discussion on Default Implementation for isDriverMetadataServiceAvailable
I think it's an improvement of the error handle in the metadata failure
case.
Not all cases need to implement it.
+1 to Andrey. That depends on which releases this feature wants to go.
Yong
On Fri, 17 May 2024 at 07:56, Andrey Yegorov wrote:
> I am ok either way.
>
> if this doesn't have default implementation it can't be included into the
> 4.17.x releases and will have to wait for 4.18 (breaking change, e.g.
> pulsar implements MetadataClientDriver interface.)
>
> OTOH if this is used for anything beyond tests I'd suggest default
> implementation to return false, but with this we can as well avoid default
> implementation and force people change code instead of failing at some
> random moment.
>
> On 2024/05/07 23:34:31 ZhangJian He wrote:
> > Hi, bookKeepers
> >
> > I've reviewed the PR that introduces `isDriverMetadataServiceAvailable`.
> I
> > have concerns about providing a default implementation that returns a
> > constant value like true, it's not a default interface like this.
> >
> > https://github.com/apache/bookkeeper/pull/4342#discussion_r1591761669
> >
> > ```
> > default E getFirstElement() {
> > return getElements().get(0);
> > }
> >
> > List getElements() {
> > }
> > ```
> >
> > - First we don't guarantee/make ABI compatible between minor releases
> > - Second, People who implement metadata drivers should implement this
> > correctly, it may lead to unintended behavior if it's not properly
> > overridden by all implementations.
> >
> > I'd suggest making this method abstract instead to avoid potential
> > misinterpretation.
> >
> > Thanks
> > ZhangJian He
> > Twitter: shoothzj
> > Wechat: shoothzj
> >
>
Re: Discussion on Default Implementation for isDriverMetadataServiceAvailable
I am ok either way.
if this doesn't have default implementation it can't be included into the
4.17.x releases and will have to wait for 4.18 (breaking change, e.g. pulsar
implements MetadataClientDriver interface.)
OTOH if this is used for anything beyond tests I'd suggest default
implementation to return false, but with this we can as well avoid default
implementation and force people change code instead of failing at some random
moment.
On 2024/05/07 23:34:31 ZhangJian He wrote:
> Hi, bookKeepers
>
> I've reviewed the PR that introduces `isDriverMetadataServiceAvailable`. I
> have concerns about providing a default implementation that returns a
> constant value like true, it's not a default interface like this.
>
> https://github.com/apache/bookkeeper/pull/4342#discussion_r1591761669
>
> ```
> default E getFirstElement() {
> return getElements().get(0);
> }
>
> List getElements() {
> }
> ```
>
> - First we don't guarantee/make ABI compatible between minor releases
> - Second, People who implement metadata drivers should implement this
> correctly, it may lead to unintended behavior if it's not properly
> overridden by all implementations.
>
> I'd suggest making this method abstract instead to avoid potential
> misinterpretation.
>
> Thanks
> ZhangJian He
> Twitter: shoothzj
> Wechat: shoothzj
>
Re: Discussion on Default Implementation for isDriverMetadataServiceAvailable
ZhangJian,
Il giorno mer 8 mag 2024 alle ore 01:34 ZhangJian He
ha scritto:
> Hi, bookKeepers
>
> I've reviewed the PR that introduces `isDriverMetadataServiceAvailable`. I
> have concerns about providing a default implementation that returns a
> constant value like true, it's not a default interface like this.
>
> https://github.com/apache/bookkeeper/pull/4342#discussion_r1591761669
>
> ```
> default E getFirstElement() {
> return getElements().get(0);
> }
>
> List getElements() {
> }
> ```
>
> - First we don't guarantee/make ABI compatible between minor releases
> - Second, People who implement metadata drivers should implement this
> correctly, it may lead to unintended behavior if it's not properly
> overridden by all implementations.
>
> I'd suggest making this method abstract instead to avoid potential
> misinterpretation.
>
It is fine to make it abstract and implement it in all of the
implementations that we maintain.
Enrico
>
> Thanks
> ZhangJian He
> Twitter: shoothzj
> Wechat: shoothzj
>
Discussion on Default Implementation for isDriverMetadataServiceAvailable
Hi, bookKeepers
I've reviewed the PR that introduces `isDriverMetadataServiceAvailable`. I
have concerns about providing a default implementation that returns a
constant value like true, it's not a default interface like this.
https://github.com/apache/bookkeeper/pull/4342#discussion_r1591761669
```
default E getFirstElement() {
return getElements().get(0);
}
List getElements() {
}
```
- First we don't guarantee/make ABI compatible between minor releases
- Second, People who implement metadata drivers should implement this
correctly, it may lead to unintended behavior if it's not properly
overridden by all implementations.
I'd suggest making this method abstract instead to avoid potential
misinterpretation.
Thanks
ZhangJian He
Twitter: shoothzj
Wechat: shoothzj
