[ovs-dev] [PATCH v6 3/3] ovsdb-idl: Force IDL retry when missing updates encountered.

[Han Zhou]

On Mon, Jun 15, 2020 at 8:18 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 6/15/20 2:41 PM, Dumitru Ceara wrote:
> > On 6/15/20 2:35 PM, Ilya Maximets wrote:
> >> On 5/28/20 2:32 PM, Dumitru Ceara wrote:
> >>> Adds a generic recovery mechanism which triggers an IDL retry with
fast
> >>> resync disabled in case the IDL has detected that it ended up in an
> >>> inconsistent state due to other bugs in the ovsdb-server/ovsdb-idl
> >>> implementation.
> >>>
> >>> CC: Andy Zhou <azhou at ovn.org>
> >>> Fixes: db2b5757328c ("lib: add monitor2 support in ovsdb-idl.")
> >>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> >>> ---
> >>>  lib/ovsdb-idl.c |   37 +++++++++++++++++++++++++++++++++----
> >>>  1 file changed, 33 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> >>> index 5abe40f..676ea2c 100644
> >>> --- a/lib/ovsdb-idl.c
> >>> +++ b/lib/ovsdb-idl.c
> >>> @@ -328,7 +328,8 @@ static bool ovsdb_idl_process_update(struct
ovsdb_idl_table *,
> >>>  static bool ovsdb_idl_process_update2(struct ovsdb_idl_table *,
> >>>                                        const struct uuid *,
> >>>                                        const char *operation,
> >>> -                                      const struct json *row);
> >>> +                                      const struct json *row,
> >>> +                                      bool *inconsistent);
> >>
> >> Hi Dumitru,
> >> The change looks good to me logically, but I don't really like that
pointer-like
> >> result returning.  I think it might be better to change result type
from boolean
> >> to enum like this:
> >>
> >> enum update2_result {
> >>     OVSDB_IDL_UPDATE2_DB_CHANGED,
> >>     OVSDB_IDL_UPDATE2_NO_CHANGES,
> >>     OVSDB_IDL_UPDATE2_INCONSISTENT,
> >> };
> >>
> >> This might make code more readable.
> >>
> >> Dumitru, Han, what do you think?
> >>
> >
> > Hi Ilya,
> >
> > Sounds good to me, should be more readable indeed. I'll change
> > ovsdb_idl_process_update/ovsdb_idl_process_update2 to return the enum
> > value as you suggested.
> >
>
> Hi Han, others,
>
> Ilya pointed out that we don't really need this last patch as we fixed
> the original issue and if we hit other bugs we will just hide them.
>
> I started having second thoughts too.
>
> Do you agree with abandoning this last patch of the series?

Agree.

Thanks,
Han

>
> Thanks,
> Dumitru
>