Hi Ben, I'm glad to hear that you find this feature useful. Below you can find my answers to your comments [SA]. I will send the updated patch set shortly. The code can be found in the same pull request: https://github.com/openvswitch/ovs/pull/110
> 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. [SA] Yes, this is how the feature behaves in this scenario. To avoid confusion, I added comments for the column and table fetch functions inside ovsdb-idl.c and also in all the wrappers that are generated for each table, as you suggested. > 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). [SA] You're right. I added a couple of functions that are called after a reconnection to handle this case. They are called from ovsdb_idl_run() when the session's seqno changes. After a reconnection, the outstanding fetch request are 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. [SA] I added more validation as you suggested. The code now first validates if the message is a reply, the result type is a json array with exactly one element (the result of a select should be ["rows": [<row>*]])), then it looks for its ID in the outstanding fetch requests. I all these conditions are met, then the json object <rows> is sent to the function that parses the reply. > 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. [SA] You are right. I change it to <row> and <rows> that are referenced in the RFC. > 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. [SA] I changed the shash to array of pointers to ovsdb_idl_column. Indeed there wasn't any reason for using a shash. > 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. Done. > 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? I changed the code a little bit and moved the common code to a single function. Now the ovsdb_idl_fetch_*() functions initialize a structure with the information of what is going to be fetched and pass it to the new function that does the actual job. This actually became useful when reissuing the request in case of a reconnection as the new function handles all the request types. > Please ask the co-authors to add their sign-offs. Done. Finally, I ran indent as described in CondingStyle.md. Regards, Sebastián -----Original Message----- From: Ben Pfaff [mailto:b...@ovn.org] Sent: Friday, March 18, 2016 11:57 AM To: Arguello, Sebastian <sebastian.argue...@hpe.com> Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/3] ovsdb-idl: Add support for on-demand columns 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
