On Tue, Feb 9, 2016 at 12:06 AM, Liran Schour <lir...@il.ibm.com> wrote:
> Andy Zhou <az...@ovn.org> wrote on 08/02/2016 09:16:18 PM:
>
> > On Mon, Feb 8, 2016 at 2:41 AM, Liran Schour <lir...@il.ibm.com> wrote:
> > I see that my reply was blocked again at the mailing list...
> >
> > Andy Zhou <az...@ovn.org> wrote on 05/02/2016 10:10:59 AM:
>
> > > On Wed, Feb 3, 2016 at 5:53 AM, Liran Schour <lir...@il.ibm.com>
> wrote:
> >
> > > This commit allows to add non-monitored columns to a monitored table.
> > > It will be used to evaluate conditions on non-monitored columns.
> > > Update notification includes only monitored columns.
> > >
> > > Signed-off-by: Liran Schour <lir...@il.ibm.com>
> > > ---
> > > ovsdb/jsonrpc-server.c | 23 +++++++++++------------
> > > ovsdb/monitor.c | 30 ++++++++++++++++++++++--------
> > > ovsdb/monitor.h | 2 +-
> > > 3 files changed, 34 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> > > index 2065702..fcda8dc 100644
> > > --- a/ovsdb/jsonrpc-server.c
> > > +++ b/ovsdb/jsonrpc-server.c
> > > @@ -1077,8 +1077,7 @@ parse_bool(struct ovsdb_parser *parser, const
> > > char *name, bool default_value)
> > > static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
> > > ovsdb_jsonrpc_parse_monitor_request(struct ovsdb_monitor *dbmon,
> > > const struct ovsdb_table *table,
> > > - const struct json
> *monitor_request,
> > > - size_t *allocated_columns)
> > > + const struct json
> *monitor_request)
> > > {
> > > const struct ovsdb_table_schema *ts = table->schema;
> > > enum ovsdb_monitor_selection select;
> > > @@ -1142,8 +1141,8 @@ ovsdb_jsonrpc_parse_monitor_request(struct
> > > ovsdb_monitor *dbmon,
> > > return ovsdb_syntax_error(columns, NULL, "%s is
> > nota valid "
> >
> > > "column name", s);
> > > }
> > > - ovsdb_monitor_add_column(dbmon, table, column, select,
> > > - allocated_columns);
> > > + ovsdb_monitor_add_column(dbmon, table, column,
> > > + select, true);
> > > }
> > > } else {
> > > struct shash_node *node;
> > > @@ -1151,8 +1150,8 @@ ovsdb_jsonrpc_parse_monitor_request(struct
> > > ovsdb_monitor *dbmon,
> > > SHASH_FOR_EACH (node, &ts->columns) {
> > > const struct ovsdb_column *column = node->data;
> > > if (column->index != OVSDB_COL_UUID) {
> > > - ovsdb_monitor_add_column(dbmon, table, column, select,
> > > - allocated_columns);
> > > + ovsdb_monitor_add_column(dbmon, table, column,
> > > + select, true);
> > > }
> > > }
> > > }
> > > @@ -1202,7 +1201,6 @@ ovsdb_jsonrpc_monitor_create(struct
> > > ovsdb_jsonrpc_session *s, struct ovsdb *db,
> > > SHASH_FOR_EACH (node, json_object(monitor_requests)) {
> > > const struct ovsdb_table *table;
> > > const char *column_name;
> > > - size_t allocated_columns;
> > > const struct json *mr_value;
> > > size_t i;
> > >
> > > @@ -1217,20 +1215,21 @@ ovsdb_jsonrpc_monitor_create(struct
> > > ovsdb_jsonrpc_session *s, struct ovsdb *db,
> > >
> > > /* Parse columns. */
> > > mr_value = node->data;
> > > - allocated_columns = 0;
> > > if (mr_value->type == JSON_ARRAY) {
> > > const struct json_array *array = &mr_value->u.array;
> > >
> > > for (i = 0; i < array->n; i++) {
> > > - error = ovsdb_jsonrpc_parse_monitor_request(
> > > - m->dbmon, table, array->elems[i],
> &allocated_columns);
> > > + error = ovsdb_jsonrpc_parse_monitor_request(m->dbmon,
> > > + table,
> > > +
> > array->elems[i]);
> > > if (error) {
> > > goto error;
> > > }
> > > }
> > > } else {
> > > - error = ovsdb_jsonrpc_parse_monitor_request(
> > > - m->dbmon, table, mr_value, &allocated_columns);
> > > + error = ovsdb_jsonrpc_parse_monitor_request(m->dbmon,
> > > + table,
> > > + mr_value);
> > > if (error) {
> > > goto error;
> > > }
> > > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> > > index 39422d9..7802560 100644
> > > --- a/ovsdb/monitor.c
> > > +++ b/ovsdb/monitor.c
> > > @@ -75,6 +75,7 @@ struct jsonrpc_monitor_node {
> > > struct ovsdb_monitor_column {
> > > const struct ovsdb_column *column;
> > > enum ovsdb_monitor_selection select;
> > > + bool monitored;
> > > };
> > >
> > > /* A row that has changed in a monitored table. */
> > > @@ -115,6 +116,8 @@ struct ovsdb_monitor_table {
> > > /* Columns being monitored. */
> > > struct ovsdb_monitor_column *columns;
> > > size_t n_columns;
> > > + size_t n_monitored_columns;
> > > + size_t allocated_columns;
> > >
> > > /* Columns in ovsdb_monitor_row have different indexes then in
> > > * ovsdb_row. This field maps between column->index to the index
> in the
> > > @@ -203,6 +206,11 @@ compare_ovsdb_monitor_column(const void *a_,
> > > const void *b_)
> > > const struct ovsdb_monitor_column *a = a_;
> > > const struct ovsdb_monitor_column *b = b_;
> > >
> > > + /* put all monitored columns at the begining */
> > > + if (a->monitored != b->monitored) {
> > > + return a->monitored ? -1 : 1;
> > > + }
> >
> > > What is the reason for above logic? Strictly speaking, this seems
> > > to be bug that may cause it caller
> > > ovsdb_monitor_table_check_duplicates() to fail to detect duplication
> > > when both monitored columns and non-monitored
> > > are mixed. It so happens that ovsdb_monitor_table_check_duplicates
> > > () are only called after monitored columns are inserted,
> > > so all columns should be monitored when this function is called --
> > > then the above logic seems redundant.
> > >
>
> > You are right. ovsdb_monitor_table_check_duplicates() should assert
> > that all columns are monitored and that's it. Will revert this
> > change and add the assertion.
> >
> > > On the other hand, condition columns are added and checked using a
> > > different (and less efficient) way. It would be great if we can somehow
> > > refactor these logics to remove some duplications.
> > >
> >
> > Both monitored and none-monitored columns are being added by
> > ovsdb_monitor_add_column(). I do not see how I can refractor it more
> > since the monitored columns are explicitly written in the
> > monitor_cond request while the none-monitored columns needed to be
> > retrieved from the added conditions.
> > However, I see now that the code missing removing unused none-
> > monitored columns on condition_update. Will add that functionality
> > to the code. Do you see how I can improve the refactoring of this code?
> >
> > How about having two column arrays per monitored table? One for
> > monitored, another for unmonitored. They can both be sorted by name
> > thus share all utility function.
> > It is probably conceptually cleaner as well.
> >
>
> We can not do that since we need single indexing for the ovsdb_datum array
> in ovsdb_row or in ovsdb_monitor_row.
That suggestion was based on the assumption that we don't have to use the
index map. Since we can not remove it, then the suggestion will not work
out. I'd agree.
>
> >
> > > From user perspective, Any duplications in conditions columns are
> > > not reported to the user, but silently ignored. Is this intentional?
> > >
> >
> > What I meant is input check. Within a single JSONRPC message, those
> > duplications are not reported. On the other hand, this is a small
> > inconsistency in implementation, so it is not a big
> > deal if you choose not implement it.
> >
> > Yes this is intentional. Condition's columns can be duplicated and
> > the user should not be aware of that. However as written above need
> > to add the code for removing unused none-monitored columns.
> >
> > So they should be reference counted in case they are used by another
> > jsonrpc session?
> >
>
> On second thought, it will be very complex to remove unused none-monitored
> columns since it will change the indexing of ovsdb_datum[]. Since the
> number of columns is not too high, I think that we can leave unused none
> monitored column without removing it for the sake of simplicity without
> making a big compromise of performance or memory usage. What do you think?
Those are the "complex data structure" I was hoping to avoid with
condition being part of a dbmon. I don't think it is ideal that we don't
reclaim unused memory for a long running server.
On the other hand, if you think it is justified in this case, then we
should add a comment why it is the case.
>
>
> >
> > > +
> > > return a->column < b->column ? -1 : a->column > b->column;
> > > }
> > >
> > > @@ -378,15 +386,15 @@ ovsdb_monitor_add_column(struct ovsdb_monitor
> *dbmon,
> > > const struct ovsdb_table *table,
> > > const struct ovsdb_column *column,
> > > enum ovsdb_monitor_selection select,
> > > - size_t *allocated_columns)
> > > + bool monitored)
> > > {
> > > struct ovsdb_monitor_table *mt;
> > > struct ovsdb_monitor_column *c;
> > >
> > > mt = shash_find_data(&dbmon->tables, table->schema->name);
> > >
> > > - if (mt->n_columns >= *allocated_columns) {
> > > - mt->columns = x2nrealloc(mt->columns, allocated_columns,
> > > + if (mt->n_columns >= mt->allocated_columns) {
> > > + mt->columns = x2nrealloc(mt->columns, &mt->allocated_columns,
> > > sizeof *mt->columns);
> > > }
> > >
> > > @@ -395,6 +403,10 @@ ovsdb_monitor_add_column(struct ovsdb_monitor
> *dbmon,
> > > c = &mt->columns[mt->n_columns++];
> > > c->column = column;
> > > c->select = select;
> > > + c->monitored = monitored;
> > > + if (monitored) {
> > > + mt->n_monitored_columns++;
> > > + }
> > > }
> > >
> > > /* Check for duplicated column names. Return the first
> > > @@ -577,7 +589,7 @@ ovsdb_monitor_compose_row_update(
> > > for (i = 0; i < mt->n_columns; i++) {
> > > const struct ovsdb_monitor_column *c = &mt->columns[i];
> > >
> > > - if (!(type & c->select)) {
> > > + if (!c->monitored || !(type & c->select)) {
> > > If monitored only columns 'select' are always OJMS_NONE, is the !
> > > c->monitored check redundant?
> > > /* We don't care about this type of change for this
> > > * particular column (but we will care about it for some
> > > * other column). */
> > > @@ -634,7 +646,7 @@ ovsdb_monitor_compose_row_update2(
> > > for (i = 0; i < mt->n_columns; i++) {
> > > const struct ovsdb_monitor_column *c = &mt->columns[i];
> > >
> > > - if (!(type & c->select)) {
> > > + if (!c->monitored || !(type & c->select)) {
> > > /* We don't care about this type of change for this
> > > * particular column (but we will care about it for
> some
> > > * other column). */
> > > @@ -1017,19 +1029,21 @@ ovsdb_monitor_table_equal(const struct
> > > ovsdb_monitor_table *a,
> > > {
> > > size_t i;
> > >
> > > + ovs_assert(b->n_columns == b->n_monitored_columns);
> > > +
> > > if ((a->table != b->table) ||
> > > (a->select != b->select) ||
> > > - (a->n_columns != b->n_columns)) {
> > > + (a->n_monitored_columns != b->n_monitored_columns)) {
> > > return false;
> > > }
> > >
> > > - for (i = 0; i < a->n_columns; i++) {
> > > + /* Compare only monitored columns that must be sorted already */
> > > + for (i = 0; i < a->n_monitored_columns; i++) {
> > > if ((a->columns[i].column != b->columns[i].column) ||
> > > (a->columns[i].select != b->columns[i].select)) {
> > > return false;
> > > }
> > > }
> > > -
> > > It seems we are going out to way trying to share monitors that
> > > shares only the monitored columns. Is this the common case?
> > > I'd think the common case will be all columns are the same
> > > (monitored or not), but the condition expression will be different.
> > > If true, then the logic may be much simpler. Please correct me if I
> > > am wrong.
> > >
>
> > Eventually the common case is that all sessions share the same
> > columns (monitored or not). However not all none-monitored columns
> > exists at the beginning of the session. The user may iteratively
> > update the condition and by that add none-monitored columns to the
> > session. So we still need to allow sharing monitor according to
> > table set and monitored columns only.
> >
> > >
> > > return true;
> > > }
> > >
> > > diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h
> > > index d6e9635..1f3dc6e 100644
> > > --- a/ovsdb/monitor.h
> > > +++ b/ovsdb/monitor.h
> > > @@ -58,7 +58,7 @@ void ovsdb_monitor_add_column(struct ovsdb_monitor
> *dbmon,
> > > const struct ovsdb_table *table,
> > > const struct ovsdb_column *column,
> > > enum ovsdb_monitor_selection select,
> > > - size_t *allocated_columns);
> > > + bool monitored);
> > >
> > > const char * OVS_WARN_UNUSED_RESULT
> > > ovsdb_monitor_table_check_duplicates(struct ovsdb_monitor *,
> > > --
> > > 2.1.4
> > >
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
