On 12/1/21 17:55, Terry Wilson wrote: > On Fri, Nov 26, 2021 at 5:38 AM Dumitru Ceara <[email protected]> wrote: >> Nit: To be consistent with the other ifs below we should probably add >> this comment: >> >> # Database contents changed. > > Will do. >
Thanks. >>> + # If 'found' is false, clear table rows for new >>> dump >>> + if not msg.result[0]: >>> + self.__clear() >> >> Can msg.result[0] be 'false'? I think so and we should call __clear() >> then and I think we won't; or am I reading this wrong? > > if not msg.result[0] is equivalent (mostly) to if msg.result[0] == > False, in which case we would call self.__clear(). > Ah, my bad, I has misread the initial code, sorry for the noise. >>> @@ -420,9 +571,16 @@ class Idl(object): >>> >>> if cond == []: >>> cond = [False] >>> - if table.condition != cond: >>> - table.condition = cond >>> - table.cond_changed = True >>> + >>> + # Compare the new condition to the last known condition >>> + if table.condition.latest != cond: >>> + table.condition.init(cond) >>> + self.cond_changed = True >>> + p = ovs.poller.Poller() >>> + p.immediate_wake() > > The poller lines here were wrong due to differences in how python-ovs > and the C code do polling. Instead, I'll add some code to Idl.wait() > to wake up if self.cond_changed. > Sounds good. >>> + return self.cond_seqno + 1 >> >> I'm not sure this is correct. In the C ovsdb-cs module we do: >> >> https://github.com/openvswitch/ovs/blob/7b8aeadd60c82f99a9d791a7df7c617254654f9d/lib/ovsdb-cs.c#L947 >> >> Which translates to expecting "self.cond_seqno + 2" if there's at least >> one condition change request in flight for one of the monitor tables. > > Ah, good point. I'll fix this. > > Thanks for the review! > Terry > Thanks! Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
