On Sat, Oct 10, 2015 at 08:19:29PM +0000, Ansari, Shad wrote:
> (Reposting this patch for RFC)
>
> Ovsdb-idl notifies a client that something changed; it does not track
> which table, row changed in what way (insert, modify or delete).
> As a result, a client has to scan or reconfigure the entire idl after
> ovsdb_idl_run(). This is presumably fine for typical ovs schemas where
> tables are relatively small. In use-cases where ovsdb is used with
> schemas that can have very large tables, the current ovsdb-idl
> notification mechanism does not appear to scale - clients need to do a
> lot of processing to determine the exact change delta.
>
> This change adds support for:
> - Table and row based change sequence numbers to record the
> most recent IDL change sequence numbers associated with insert,
> modify or delete update on that table or row.
> - Change tracking of specific columns. This ensures that changed
> rows (inserted, modified, deleted) that have tracked columns, are
> tracked by IDL. The client can directly access the changed rows
> with get_first, get_next operations without the need to scan the
> entire table.
> The tracking functionality is not enabled by default and needs to
> be turned on per-column by the client after ovsdb_idl_create()
> and before ovsdb_idl_run().
>
> /* Example Usage */
> {
> idl = ovsdb_idl_create(...);
> /* Track specific columns */
> ovsdb_idl_track_add_column(idl, column);
> /* Or, track all columns */
> ovsdb_idl_track_add_all(idl);
> for (;;) {
> ovsdb_idl_run(idl);
> seqno = ovsdb_idl_get_seqno(idl);
> /* Process only the changed rows in Table FOO */
> FOO_FOR_EACH_TRACKED(row, idl) {
> /* Determine the type of change from the row seqnos */
> if (foo_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > seqno)) {
> /* row is deleted */
> } else if {foo_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) >
> seqno))
> /* row is modified */
> } else if {foo_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) >
> seqno))
> /* row is inserted */
> }
> }
> /* All changes processed - clear the change track */
> ovsdb_idl_track_clear(idl);
> }
> }
>
> Signed-off-by: Shad Ansari <shad.ans...@hp.com>
First, I like this. Thank you for doing the work.
I see a few style errors, e.g. the { should be on a line of its own
here:
static bool
ovsdb_idl_track_is_set(struct ovsdb_idl_table *table) {
ovsdb_idl_track_is_set() seems expensive given that it is being executed
on every row destroy.
I'm not really pleased with the idea that this creates two different
modes for ovsdb_idl_row_destroy() that have significantly different
behavior and need to be tested separately. In practice, that testing
will be incomplete (I note that you added tests--thank you!--but of
course ovs-vswitchd is only going to use one mode or the other, and it's
the main user of ovsdb-idl in the OVS testsuite).
So, I'd be happier if we could reduce this to a single mode. That seems
reasonable to me: have ovsdb_idl_row_destroy() always queue up a row for
tracking, then at a high level (the end of ovsdb_idl_run() may be a
reasonable choice) automatically destroy the tracking queues for the
tables which the client doesn't care to track. Does that sound OK to
you?
I'm a little nervous about ovsdb_idl_track_add_column(). It seems to
have a pitfall in that it does nothing, silently, if the column isn't
already set up for alerts. It seems to me that it would be harder to
misuse if it either turned on alerts automatically or if it
assert-failed if they were not already turned on.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
