Andy Zhou <[email protected]> wrote on 11/01/2016 10:57:30 PM:
>
> On Mon, Jan 11, 2016 at 5:05 AM, Liran Schour <[email protected]> wrote:
> Andy Zhou <[email protected]> wrote on 09/01/2016 02:02:32 AM:
> >
> > On Fri, Jan 8, 2016 at 8:58 AM, Liran Schour <[email protected]>
wrote:
> > Andy Zhou <[email protected]> wrote on 07/01/2016 01:04:17 AM:
> > >
> > > 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.
> > >
> >
> > In case of changing conditions, there is a real need for that. I was
> > getting to it from the perspective of OVN however I believe there are
> > others use cases for that functionality. In case of changing the
monitored
> > columns, I am not sure there is a real need for that. If there is, we
can
> > change "monitor_cond_change" to a more generic "monitor_updtae" that
will
> > be able to change the monitored columns. However that will add
complexity
> > to the code because columns unlike conditions are being held by
> > ovsdb_monitor and they are common to all jsonrpc monitor session's
using
> > this ovsdb_monitor. Changing the columns in the middle will force us
to
> > move this jsonrpc monitor session to a new ovsdb_monitor with the new
> > columns. In case of the conditions, they are specific per jsonrpc
monitor
> > session from the start.
> > If there is a real need for changing the monitored columns I can add
this
> > functionality to the patch series. Tell me what do you think?
>
> > Purely from protocol stand point, it would be nice to avoid
> > introducing new methods
> > if we can avoid it. If there is not an immediate need for column
> > changes, we can
> > restrict our implementation to condition changes only to begin
> > with. The implementation
> > can simply nack any monitor_updates that contains non-empty
> > <monitor-request>.
> >
> I still think there is a difference between the conditions and
> columns and we should not allow changing columns during session like
> we trying to allow regarding to conditions. Columns is a more static
> element, we know all the columns from the start.
>
> I don't think the difference is fundamental to the protocol.
> In practice, change conditions has immediate use case. Adding
> columns (or tables) may be useful down the road. It would be nice to
allow
> the wire protocol to accommodate it for such use cases down the road.
>
>
> In my opinion if a user wants to change the monitored columns it
> should cancel the existing monitor session and start a new one with
> the new columns.
>
> This would have the same issues of cancelling a monitored session
> for where clause changes.
>
> It is different in case of conditions. It is a trivial use case to
> iteratively change the conditions to change the monitored rows.
> Conditions are a more dynamic issue the columns.
> This may be more implementation specific.
>
> Anyhow it seems like a specification matter and we already had a
> review over the spec in the past. Maybe we should raise this
> question to re-approve the spec before I will implement it.
> What do you think?
> I agree. Sorry this issue did not occur to me during the spec review.
Following the review we have open issues regarding the spec of
"monitor_cond_change" method:
1. Do we want to allow the clients to change monitored tables an columns
via "monitor_cond_change" (we can call it "monitor_update" then)?
At my opinion in case that the client needs to change monitored tables and
columns it should cancel the existing monitor session and create a new one
with the desired columns and tables. It is not the case regarding to
conditions , since conditions are more a dynamic element regarding values
of columns, a trivial use case will be to iteratively change the
conditions and by that change the monitored rows. If we choose to allow
the client to change the monitored tables and columns we also add a
complexity to the code since tables and columns are being shared with
multiple client's RPC sessions.
2. Do we want to allow "monitor_cond_change" to change the json-value
associated with the monitor session?
I add here the existing specification of the method:
Monitor_cond_change
The "monitor_cond_change" request enables a client to change an existing
conditional replication of the database by specifying a new set of
conditions for each replicated table.
The request object has the following members:
"method": "monitor_cond_change"
"params": [<json-value>, <monitor-cond-change-requests>]
"id": <nonnull-json-value>
The <json-value> parameter should have a value of an existing
conditional monitoring session from this client. The
<monitor-cond-change-requests> object maps the name of the table to an
array of <monitor-cond-change-request>.
Each <monitor-cond-change-request> is an object with the following
members:
added: [<condition>*]
new set of conditions to be added to the existing set of
conditions for this table - optional.
removed: [<condition>*]
existing conditions to be removed from replication -
optional.
<condition> is specified in Section 5.1 in the RFC with the following
change: A condition can be either a 3-element JSON array as
described in the RFC or a boolean value. In case of an empty array an
implicit true boolean value will be considered, and all rows will be
monitored.
The response object has the following members:
"result": <table-updates>
"error": null
"id": same "id" as request
The <table-updates2> object is described in detail in Section 4.1.14 in
the RFC. If insert contents are requested by origin monitor_cond
request, <table-updates2> will contain any newly matched rows by
"added" conditions. If deleted contents are requested by origin monitor
request, <table-updates2> will contain any matched rows by "removed"
conditions.
Changes according to the new conditions are automatically sent to the
client using the "update2" monitor notification. Updates as a result
of a condition change, will be sent only after the client received a
response to the "monitor_cond_change" request.
Thanks,
- Liran
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
