Andy Zhou <az...@ovn.org> wrote on 23/02/2016 11:21:31 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 agree that this can happen in theory, especially when the jsonrpc
> client connection is slow, or stalled for some reason. I am just not
> not sure this actually can happen in a unit test environment.
>
>
> 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...
>
> True. Do you see an issue outside of unit test? It would be good to
> get Ben's perspective on this also since he will review those
> patches as well.
>
No do not see other issues. The patch is good and will reduce some
computation on the server side.
>
>
> 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>
> Thanks for the review!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
