[ovs-dev] [PATCH monitor_cond 00/12] Implement conditional monitoring

[Andy Zhou]

I have some comments on the patch series.

Instead of "monitor_cond_change", why not just have a more generic
"monitor_update" message, so we can update
monitor columns as well as conditions.

Instead of reusing monitor id,  when "monitor_update" can also change to
use a new value.  Since update message
embeds the monitor_id, a client can tell the version of monitor  (and
conditions) applied for the update. Of course, monitor
cancellation needs to use the new id.

Should any columns within schema be allowed in condition? or only the ones
being monitored?
>From protocol viewpoint, it would make sense to allow any column.

I see that conditions are applied when generating updates.  Could this lead
to inconsistency when fast and slow connections receives different content
when condition changes?

Noticed that tabs are used in some files, for example, condition.[ch]

It may make the code simpler if we keep the 3 term "condition" also for
true and false functions, but normalize the column and data to some known
values.

ovsdb-server man page section 4.1.13 says monitor_cond_change should use
<table-update>.  Is this a typo? We should be using <table-update2> instead.



On Tue, Jan 5, 2016 at 1:54 PM, Andy Zhou <azhou at ovn.org> wrote:

>
>
> On Tue, Jan 5, 2016 at 5:13 AM, Liran Schour <lirans at il.ibm.com> wrote:
>
>> This patch series implements conditional monitoring by introducing an
>> OVSDB
>> RFC extension with 2 new JSON-RPC methods: "monitor_cond" and
>> "monitor_cond_change". Specification of this extension is defined in the
>> ovsdb-server (1) man page.
>> Monitor2 is now merged into monitor_cond. A monitor_cond session with an
>> empty
>> condition, will behave exactly like monitor2 and will get update2
>> notifications.
>>
>> Thanks for posting. I will take a closer look. When applying patches,
> git-am complained about
> some white space issues:
>
> Description: [ovs-dev,monitor_cond,02/12] ovsdb: add conditions utilities
> to support monitor_cond
> Applying: ovsdb: add conditions utilities to support monitor_cond
> /home/azhou/projs/ovs-review/ovs/.git/rebase-apply/patch:141: trailing
> whitespace.
>     field = !columns_index_map ?
> warning: 1 line adds whitespace errors.
>
> Description: [ovs-dev,monitor_cond,04/12] ovsdb-client: support
> monitor-cond
> Applying: ovsdb-client: support monitor-cond
> /home/azhou/projs/ovs-review/ovs/.git/rebase-apply/patch:53: trailing
> whitespace.
> all rows will be monitored. If at least one \fIcolumn\fR is specified,
> only those
> warning: 1 line adds whitespace errors.
>
> Description: [ovs-dev,monitor_cond,07/12] ovsdb: enable jsonrpc-server to
> service "monitor_cond_change" request
> Applying: ovsdb: enable jsonrpc-server to service "monitor_cond_change"
> request
> /home/azhou/projs/ovs-review/ovs/.git/rebase-apply/patch:78: trailing
> whitespace.
>
> /home/azhou/projs/ovs-review/ovs/.git/rebase-apply/patch:180: trailing
> whitespace.
>
> warning: 2 lines add whitespace errors.
>
>
>
>> Note: With this patch the json cache will be used only for monitor
>> sessions
>> with an empty condition. We can think on implementing a per row json
>> cache that
>> will be matched against condition clauses in the future.
>>
>>
>> Can we add conditions as part of cache selector?
>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>