"dev" <dev-boun...@openvswitch.org> wrote on 23/02/2016 02:28:12 AM:
> dbmon's changes should be stored with the next transaction number, > rather than the current transaction number. This bug causes the > changes of a transaction stored in a monitor to be unnoticed by > the jsonrpc connections that is responsible for flush the monitor > content. > > However, the bug was not noticed until it was exposed by a later > optimization patch: "avoid unnecessary call to ovsdb_monitor_get_update()." > The lack of optimization means that the update is still generated > when 'unflushed' equals to n_transactions + 1, which should have > indicated the monitor has been flushed already. > > Signed-off-by: Andy Zhou <az...@ovn.org> > This patch looks good. However the essence of this change is that in case of OVSDB_CHANGES_REQUIRE_INTERNAL_UPDATE n_transcations will be incremented. That prevents you from seeing the inconsistency in the tests I mentioned here: http://openvswitch.org/pipermail/dev/2016-February/066501.html I still think that it is an issue. If you do insert X and then delete X the client might not know about X ever or get a notification about X. We can not be sure. What we know for sure that X will not be in the client's replica at the end... Look at the following code from monitor.c: ovsdb_monitor_changes_update(const struct ovsdb_row *old, const struct ovsdb_row *new, const struct ovsdb_monitor_table *mt, struct ovsdb_monitor_changes *changes) { const struct uuid *uuid = ovsdb_row_get_uuid(new ? new : old); struct ovsdb_monitor_row *change; change = ovsdb_monitor_changes_row_find(changes, uuid); if (!change) { change = xzalloc(sizeof *change); hmap_insert(&changes->rows, &change->hmap_node, uuid_hash(uuid)); change->uuid = *uuid; change->old = clone_monitor_row_data(mt, old); change->new = clone_monitor_row_data(mt, new); } else { if (new) { update_monitor_row_data(mt, new, change->new); } else { free_monitor_row_data(mt, change->new); change->new = NULL; if (!change->old) { /* This row was added then deleted. Forget about it. */ hmap_remove(&changes->rows, &change->hmap_node); free(change); -----> Here we remove the entire change and the client never been notified about that.... } } } } Otherwise this patch series looks good and do fix a problem. Acked-by: Liran Schour <lir...@il.ibm.com> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
