You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Venkateswara Rao Jujjuri <ju...@gmail.com> on 2019/06/12 17:26:21 UTC
Bug in OnClusterChanged() ?
I am looking at onClusterChanged() in
TopologyAwareEnsemblePlacementPolicy.java
and I believe we don't handle the following case.
1. Bookie Became RO. We remove this from known bookies and add it to
readOnlyBookies.
2. Same bookie went down; Now the arguments, writableBookies has no change,
and readOnlyBookies is NULL.
At this point leftBookies, joinedBookies and deadBookies all get evaluated
to NULL.
Also the following check doesn't even update readOnlyBookies
if (!readOnlyBookies.isEmpty()) {
this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
}
So we will continue to have down bookie as part of our readOnlyBookie.
Am I missing something?
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
Re: Bug in OnClusterChanged() ?
Posted by Enrico Olivelli <eo...@gmail.com>.
Il sab 13 lug 2019, 21:28 Venkateswara Rao Jujjuri <ju...@gmail.com> ha
scritto:
> Question: Any idea why we have a special check if we receive
> emptyreadOnlyBookies we need to ignore?
> It is possible that we can have no readonly bookies. Why not
> unconditionally take what came from onClusterChanged
> just like writableBookies?
>
Thinking out loud...
What happens if you change that block of code and run the tests? If your
idea is valid at least we won't see failure.
If you get a failure you will see the case in which the check is needed
Enrico
> if (!readOnlyBookies.isEmpty()) {
> this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
> }
>
>
> On Fri, Jul 12, 2019 at 9:25 AM Venkateswara Rao Jujjuri <
> jujjuri@gmail.com>
> wrote:
>
> > Not yet; may be the scope/window is extremely tiny. For this to be a
> > problem, only one node has to become RO->Offline.
> > If more than one node becomes RO then we don't have this issue. Not sure
> > if anyone else looked at it yet.
> >
> > On Fri, Jul 12, 2019 at 9:03 AM Enrico Olivelli <eo...@gmail.com>
> > wrote:
> >
> >> Does anyone have a chance to take a look?
> >>
> >> Enrico
> >>
> >> Il mer 12 giu 2019, 19:26 Venkateswara Rao Jujjuri <ju...@gmail.com>
> ha
> >> scritto:
> >>
> >> > I am looking at onClusterChanged() in
> >> > TopologyAwareEnsemblePlacementPolicy.java
> >> > and I believe we don't handle the following case.
> >> >
> >> > 1. Bookie Became RO. We remove this from known bookies and add it to
> >> > readOnlyBookies.
> >> > 2. Same bookie went down; Now the arguments, writableBookies has no
> >> change,
> >> > and readOnlyBookies is NULL.
> >> > At this point leftBookies, joinedBookies and deadBookies all get
> >> evaluated
> >> > to NULL.
> >> > Also the following check doesn't even update readOnlyBookies
> >> >
> >> > if (!readOnlyBookies.isEmpty()) {
> >> > this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
> >> > }
> >> >
> >> > So we will continue to have down bookie as part of our readOnlyBookie.
> >> >
> >> > Am I missing something?
> >> >
> >> > --
> >> > Jvrao
> >> > ---
> >> > First they ignore you, then they laugh at you, then they fight you,
> then
> >> > you win. - Mahatma Gandhi
> >> >
> >>
> >
> >
> > --
> > Jvrao
> > ---
> > First they ignore you, then they laugh at you, then they fight you, then
> > you win. - Mahatma Gandhi
> >
> >
> >
>
> --
> Jvrao
> ---
> First they ignore you, then they laugh at you, then they fight you, then
> you win. - Mahatma Gandhi
>
Re: Bug in OnClusterChanged() ?
Posted by Venkateswara Rao Jujjuri <ju...@gmail.com>.
Question: Any idea why we have a special check if we receive
emptyreadOnlyBookies we need to ignore?
It is possible that we can have no readonly bookies. Why not
unconditionally take what came from onClusterChanged
just like writableBookies?
if (!readOnlyBookies.isEmpty()) {
this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
}
On Fri, Jul 12, 2019 at 9:25 AM Venkateswara Rao Jujjuri <ju...@gmail.com>
wrote:
> Not yet; may be the scope/window is extremely tiny. For this to be a
> problem, only one node has to become RO->Offline.
> If more than one node becomes RO then we don't have this issue. Not sure
> if anyone else looked at it yet.
>
> On Fri, Jul 12, 2019 at 9:03 AM Enrico Olivelli <eo...@gmail.com>
> wrote:
>
>> Does anyone have a chance to take a look?
>>
>> Enrico
>>
>> Il mer 12 giu 2019, 19:26 Venkateswara Rao Jujjuri <ju...@gmail.com> ha
>> scritto:
>>
>> > I am looking at onClusterChanged() in
>> > TopologyAwareEnsemblePlacementPolicy.java
>> > and I believe we don't handle the following case.
>> >
>> > 1. Bookie Became RO. We remove this from known bookies and add it to
>> > readOnlyBookies.
>> > 2. Same bookie went down; Now the arguments, writableBookies has no
>> change,
>> > and readOnlyBookies is NULL.
>> > At this point leftBookies, joinedBookies and deadBookies all get
>> evaluated
>> > to NULL.
>> > Also the following check doesn't even update readOnlyBookies
>> >
>> > if (!readOnlyBookies.isEmpty()) {
>> > this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
>> > }
>> >
>> > So we will continue to have down bookie as part of our readOnlyBookie.
>> >
>> > Am I missing something?
>> >
>> > --
>> > Jvrao
>> > ---
>> > First they ignore you, then they laugh at you, then they fight you, then
>> > you win. - Mahatma Gandhi
>> >
>>
>
>
> --
> Jvrao
> ---
> First they ignore you, then they laugh at you, then they fight you, then
> you win. - Mahatma Gandhi
>
>
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
Re: Bug in OnClusterChanged() ?
Posted by Venkateswara Rao Jujjuri <ju...@gmail.com>.
Not yet; may be the scope/window is extremely tiny. For this to be a
problem, only one node has to become RO->Offline.
If more than one node becomes RO then we don't have this issue. Not sure if
anyone else looked at it yet.
On Fri, Jul 12, 2019 at 9:03 AM Enrico Olivelli <eo...@gmail.com> wrote:
> Does anyone have a chance to take a look?
>
> Enrico
>
> Il mer 12 giu 2019, 19:26 Venkateswara Rao Jujjuri <ju...@gmail.com> ha
> scritto:
>
> > I am looking at onClusterChanged() in
> > TopologyAwareEnsemblePlacementPolicy.java
> > and I believe we don't handle the following case.
> >
> > 1. Bookie Became RO. We remove this from known bookies and add it to
> > readOnlyBookies.
> > 2. Same bookie went down; Now the arguments, writableBookies has no
> change,
> > and readOnlyBookies is NULL.
> > At this point leftBookies, joinedBookies and deadBookies all get
> evaluated
> > to NULL.
> > Also the following check doesn't even update readOnlyBookies
> >
> > if (!readOnlyBookies.isEmpty()) {
> > this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
> > }
> >
> > So we will continue to have down bookie as part of our readOnlyBookie.
> >
> > Am I missing something?
> >
> > --
> > Jvrao
> > ---
> > First they ignore you, then they laugh at you, then they fight you, then
> > you win. - Mahatma Gandhi
> >
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
Re: Bug in OnClusterChanged() ?
Posted by Enrico Olivelli <eo...@gmail.com>.
Does anyone have a chance to take a look?
Enrico
Il mer 12 giu 2019, 19:26 Venkateswara Rao Jujjuri <ju...@gmail.com> ha
scritto:
> I am looking at onClusterChanged() in
> TopologyAwareEnsemblePlacementPolicy.java
> and I believe we don't handle the following case.
>
> 1. Bookie Became RO. We remove this from known bookies and add it to
> readOnlyBookies.
> 2. Same bookie went down; Now the arguments, writableBookies has no change,
> and readOnlyBookies is NULL.
> At this point leftBookies, joinedBookies and deadBookies all get evaluated
> to NULL.
> Also the following check doesn't even update readOnlyBookies
>
> if (!readOnlyBookies.isEmpty()) {
> this.readOnlyBookies = ImmutableSet.copyOf(readOnlyBookies);
> }
>
> So we will continue to have down bookie as part of our readOnlyBookie.
>
> Am I missing something?
>
> --
> Jvrao
> ---
> First they ignore you, then they laugh at you, then they fight you, then
> you win. - Mahatma Gandhi
>