[ovs-dev] [ovsdb monitors 8/8] ovsdb: Extend "monitor" to select different operations in a single table.

[Jesse Gross]

On Wed, Jun 30, 2010 at 4:49 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> -<monitor-requests> is an object that maps from a table name to a
> -<monitor-request>.
> +<monitor-requests> is an object.  Each name in the object is a table
>

I'm not sure that saying "<x> is an object" is overly useful as a sentence.


> +of tables within database <db-name>.  Each element of
> +<monitor-request> specifies a table to be replicated.  The JSON-RPC
>

Doesn't each <monitor-request> specify a column, not a table?  Or should
this be <monitor-requests>?


> +response to the "monitor" includes the initial contents of each table.
>

It only includes the initial contents if you ask for them, right? (well, if
you don't turn it off)


> +    /* This is the union (exclusive-OR) of the 'select' values in all of
> the
> +     * members of 'columns' below. */
>     enum ovsdb_jsonrpc_monitor_selection select;
>

I know that overlapping column sets aren't allowed and we will reject them
but the use of exclusive OR seems a little bit weird to me here.  It makes
me think that if you do have an overlap we do something weird with it.

@@ -705,8 +831,9 @@ ovsdb_jsonrpc_monitor_create(struct
> ovsdb_jsonrpc_session *s,
>     SHASH_FOR_EACH (node, json_object(monitor_requests)) {
>         const struct ovsdb_table *table;
>         struct ovsdb_jsonrpc_monitor_table *mt;
> -        const struct json *columns_json, *select_json;
> -        struct ovsdb_parser parser;
> +        size_t allocated_columns;
> +        const struct json *value;
>

I think maybe a less generic variable name than "value" would be good here.


> -.IP "\fBmonitor\fI server database table\fR
> [\fIcolumn\fR[\fB,\fIcolumn\fR]...] [\fIselect\fR[\fB,\fIselect\fR]...]"
> +.IP "\fBmonitor\fI server database table\fR
> [\fIcolumn\fR[\fB,\fIcolumn\fR]...]"
>  Connects to \fIserver\fR and monitors the contents of \fItable\fR in
>  \fIdatabase\fR.  By default, the initial contents of \fItable\fR are
>  printed, followed by each change as it occurs.  If at least one
> -\fIcolumn\fR is specified, only those columns are monitored.  If at
> -least one \fIselect\fR is specified, they are interpreted as follows:
> +\fIcolumn\fR is specified, only those columns are monitored.  The
> +following \fIcolumn\fR names have special meanings:


This doesn't really make it clear to me that you can have multiple groups of
columns, each with their own reporting parameters.

Otherwise, the rest of this series looks good to me.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100630/131a8d8b/attachment-0003.html>