Ben Pfaff <b...@ovn.org> wrote on 02/06/2016 03:07:08 AM: > On Tue, May 17, 2016 at 05:27:02PM +0300, Liran Schour wrote: > > Hold session's conditions in ovsdb_monitor_session_condition. Pass it > > to ovsdb_monitor for generating "update2" notifications. > > Add functions that can generate "update2" notification for a > > "monitor_cond" session. > > JSON cache is enabled only for session's with true condition only. > > "monitor_cond" and "monitor_cond_change" are RFC 7047 extensions > > described by ovsdb-server(1) manpage. > > Thanks for the updated patch. > > Please make ovsdb_monitor_session_condition_destroy() accept a null > pointer argument and treat it as a no-op, and then remove the check from > the caller, following CodingStyle: > > Functions that destroy an instance of a dynamically-allocated type > should accept and ignore a null pointer argument. Code that calls > such a function (including the C standard library function free()) > should omit a null-pointer check. We find that this usually makes > code easier to read. >
OK. > In the documentation, s/Replay/Reply/, s/deescribed/described/, > s/origin/original/. > Will fix that. > In the documentation, there's a reference to [<conditions>*] but that I > think that should be [<condition>*] (without the 's'). The updated > meaning of <condition> is described in a couple of places, but I think > that instead we should just describe an update to the definition of > <condition>, for whatever section defines <condition> (and probably > should do that for the commit a few patches ago that actually updates > the condition parser). > Will fix that and move the <condition> specification to the former patch. > There's a stray comma on a line by itself in the documentation, here: > > +contains the contents of the tables for which "initial" rows > are selected. > +If no tables initial contents are requested, then "result" is > an empty object. > +, > +.IP > Right. Will fix that. > The parentheticals look funny in the documentation here, I'd suggest > just removing them: > > +.IP \(bu > +If "insert" is omitted or true, "update" notifications are sent > for rows newly > +inserted into the table that match conditions or for rows > modified in the table > +so that their old version does not match the condition and new > version does. > +(new row in the client's replica table) > +.IP \(bu > +If "delete" is omitted or true, "update" notifications are sent > for rows deleted > +from the table that match conditions or for rows modified in > the table so that > +their old version does match the conditions and new version > does not. (deleted row > +in the client's replica) > Will remove that. > In the documentation for monitor_cond_update, I'm not sure there's value > in describing what's currently unsupported. > Will move the documentation of monitor_cond_update to the patch that implements this. > We already have some messages with "update" in their names with quite > different semantics. What if we renamed monitor_cond_update to > monitor_cond_change or similar? Then there would be less confusion. > Will change monitor_cond_update to monitor_cond_change. > I don't understand the design for how the monitor_cond_update replies > are meant to be handled. The client's goal is probably to know how the > database content as seen through the new condition differs from the > database content as seen through the old condition; let's call this > difference D, just to be clear. The documentation says such an update > is only sent after the monitor_cond_update reply: > > Updates as a result of a condition change, will be sent only after > the client received a response to the "monitor_cond_update" request. > > With that design, I don't see how the client can find out whether D is > empty or nonempty without waiting for an indefinite time or for some > further change to occur. For example, if D is empty and the database > does not change, then the client will wait forever to receive a new > update2 notification. The client can't simply assume that D is empty; > after all, what if the database is just slow? To solve the problem, I > would suggest that we change the design, so that the documentation would > be more like: > > An update, if any, as a result of a condition change, will be sent > to the client before the reply to the "monitor_cond_update" request. > Will change the code and documentation to follow the paragraph above. Thanks for reviewing the code. - Liran _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
