Terry Wilson via dev <[email protected]> writes:
> The Python IDL's row_update2 "modify" handling always generated a
> ROW_UPDATE notification and bumped change_seqno, even when the diff only
> touched columns registered without an alert. This diverged from the C
> IDL, whose ovsdb_idl_modify_row_by_diff() reports whether a monitored
> (alerted) column actually changed and only then treats the row as
> updated.
>
> Make _apply_diff() report whether an alerted column changed, and have
> _process_update2()'s "modify" branch notify only in that case, matching
> the C behavior. This is also a prerequisite for faithfully porting the
> inconsistency detection that keys off modify_row_by_diff()'s return
> value.
>
> There is no single C commit to port here: the alert gating originates in
> C commit c547535a7 ("ovsdb-idl: Make it possible to omit or pay less
> attention to columns.") applied to the row-update2 diff path added in
> db2b57573 ("lib: add monitor2 support in ovsdb-idl."). This commit
> brings the Python IDL in line with that long-standing C behavior.
>
> De-mangle __apply_diff/__process_update2 to single-underscore names as
> they are modified here.
>
> Assisted-by: Claude Opus 4.8 <[email protected]>
> Signed-off-by: Terry Wilson <[email protected]>
> ---
This is an AI generated review of your patch. A human has looked at the
results and deemed any concerns as plausible.
Reviewed commit 6564fada ("python: idl: Notify on update2 diff only if
alerted column changes."), touching python/ovs/db/idl.py only. The
change makes `_apply_diff()` return `(old_row, changed)` where `changed`
is set only when a datum actually differs from its pre-diff copy *and*
the column has `alert` set, and gates the ROW_UPDATE notice (and hence
the change_seqno bump in `__do_parse_update`) on that flag. It also
de-mangles `__apply_diff`/`__process_update2` to single underscore.
Verification performed:
- Read `_process_update2`, `_apply_diff`, `__row_update`, and
`__do_parse_update` at the commit revision (git show of the file). The
name rename has no other callers: repo-wide grep for
`apply_diff|process_update2` shows only C-side
`ovsdb_datum_apply_diff*` hits plus idl.py itself; no Python
subclasses or tests reference the old mangled names
(`_Idl__apply_diff`).
- Verified the comment's claim about in-place mutation: `Datum.diff()`
(python/ovs/db/data.py:418-429 at this revision) mutates `self.values`
for sets/maps and returns `self`, so the pre-diff `copy()` is indeed
required for a meaningful comparison; `Datum.copy()` (data.py:302)
shallow-copies the values dict, which is safe because `Atom` is
immutable.
- Confirmed `column.alert` always exists (`IdlColumn.__init__` sets it
True at idl.py:174; server-table columns are forced True near line
924), so `column.alert` cannot raise AttributeError, and the gating
mirrors `__row_update` (idl.py:1271 at this revision).
- Compared with C: `ovsdb_idl_row_change()` in lib/ovsdb-idl.c gates the
alert check identically for the non-xor path, but for the xor/diff
path it sets `datum_changed = true` whenever the diff applies without
a parse error, without comparing old vs new values.
Concern:
> old = row._data[column_name].copy()
> old_row[column_name] = old
> datum = row._data[column_name].diff(datum_diff)
> row._data[column_name] = datum
> if datum != old and column.alert:
> changed = True
The `datum != old` requirement makes Python slightly stricter than the C
IDL for the diff path. In C (`ovsdb_idl_row_change()`, lib/ovsdb-idl.c
around line 1902-1910), the xor branch sets `datum_changed = true` on
any successful `ovsdb_datum_apply_diff_in_place()` regardless of whether
the resulting datum actually differs, so a diff that touches an alert
column but is a value no-op still counts as a change there. Here, such a
diff would not set `changed`, so no ROW_UPDATE notice and no
change_seqno bump. In practice servers compute diffs from real changes,
so a no-op diff should not occur, and the Python behavior arguably
matches the intent better (and matches `__row_update`'s equality
gating); but it means the commit does not match C exactly on this edge,
which matters for the stated follow-up work ("faithfully porting the
inconsistency detection that keys off modify_row_by_diff()'s return
value") if that port assumes C's "diff applied = changed"
semantics. Minor documentation/semantics nit rather than a practical
bug.
Two smaller observations, no action needed: (1) `_apply_diff` is now a
single-underscore method with a changed return signature (dict ->
tuple); any out-of-tree subclass overriding the old name-mangled
`__apply_diff` would silently stop being called, but nothing in this
repo does. (2) For scalar columns whose diff parses to an equal value,
the new unconditional `row._data[column_name] = datum` materializes an
explicit entry in the lazy `ColumnDefaultDict` where the old code left
the default lazy; values are equal and
`ColumnDefaultDict.__contains__`/`keys()` cover all columns, so behavior
is unchanged.
Verdict: PASS
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
