On Tue, Mar 01, 2016 at 05:42:19PM +0000, Arguello, Sebastian wrote:
> The IDL only supports reading from columns that are being monitored.
> In the case where the column represent a frequently changing entity (e.g.
> counter),
> and the reads are relatively infrequent (e.g. CLI client), there is a
> significant overhead in replication.
>
> This patch introduces a new column mode called OVSDB_IDL_ON_DEMAND.
> An on-demand column will have its default value if it has never been updated.
> This can happen if: 1) the user has not called explicitly a fetch operation
> over it or 2) the server reply with the actual value has not been
> received/processed (i.e. ovsdb_idl_run() has not been called). The on-demand
> columns will keep the last value received from the OVSDB server.
>
> The on-demand columns are not updated by the IDL automatically, they are
> updated when the IDL user asks it by the calling one of the new fetching
> functions that were added to the IDL. When this happens, the IDL sends a
> select
> operation to request the data from the server. After calling ovsdb_idl_run(),
> the IDL updates the replica with the information received from the server.
>
> With this new column mode, the state of the replica could diverge from the
> state of the database, as some of the columns could be outdated. The process
> using the IDL is responsible for requesting the information before using it.
>
> The main user visible changes in this patch are:
> - There is a new function that adds on-demand columns:
> ovsdb_idl_add_on_demand_column()
> - Functions for fetching a cells (columns for specific rows),
> columns, and table were added: ovsdb_idl_fetch_row(),
> ovsdb_idl_fetch_column(), and ovsdb_idl_fetch_table()
> - Functions for verifying if the fetch requests of on-demand columns
> were processed were added: ovsdb_idl_is_row_fetch_pending(),
> ovsdb_idl_is_column_fetch_pending(), ovsdb_idl_is_table_fetch_pending()
> - When an on-demand column is updated, the IDL seqno is changed as well
>
> Note that the Python IDL already has a feature similar to this called
> Read-only columns.
>
> Signed-off-by: Sebastian Arguello <sebastian.argue...@hpe.com>
> ---
> This is the pull request with this change:
> https://github.com/openvswitch/ovs/pull/110
Thanks for working on this. I think it will have useful applications.
I have some comments.
I believe that there is a race between requesting a fetch for a column
or a table and insertion of a new row: if client 1 requests a fetch for
a column or table and then client 2 inserts a new row, then client 1
will see a row without the requested data. I think that's just the way
things work and users of the IDL need to expect it. If so, I think that
it should be documented in the comments for the functions in question,
so that users of the IDL don't get any surprises.
I don't see anything to handle the case where the database connection
drops. If that happens, then any outstanding requests will never
receive replies, so all the pending-fetch counters need to be reset and
the outstanding fetch requests need to be discarded (or, alternatively,
they could be re-issued).
The code in ovsdb_idl_run() and ovsdb_idl_parse_fetch_reply() seems to
assume that if the JSON hash value can be found in the outstanding fetch
requests, then there's no need to compare the actual JSON. I don't see
why this is safe.
The log messages refer to <fetch_reply>, but that's not one of the
nonterminals in the context-free grammar for OVSDB as are the other uses
of the <name> notation.
It's not clear to me why struct ovsdb_idl_fetch_node's 'columns' member
is a shash instead of just an array of pointers to ovsdb_idl_column. It
doesn't look to me like anything ever uses the 'name' part of each shash
node to do a lookup; instead, the only use of this data structure is to
iterate over the set of columns.
I see a few log messages that use raw errno values, like this:
VLOG_WARN_RL(&syntax_rl,
"Error while sending column fetch request (%d)", status);
Please use ovs_strerror() to convert these to human-readable messages.
The ovsdb_idl_fetch_*() functions have some code in common. Is there a
way to factor some of this out, in a natural way, to reduce the amount
of common code?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
