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.
In the documentation, s/Replay/Reply/, s/deescribed/described/,
s/origin/original/.
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).
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
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)
In the documentation for monitor_cond_update, I'm not sure there's value
in describing what's currently unsupported.
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.
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.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
