Ben Pfaff <b...@ovn.org> wrote on 12/01/2016 08:22:26 PM: > > On Tue, Jan 12, 2016 at 10:12:05AM +0200, Liran Schour wrote: > > Andy Zhou <az...@ovn.org> wrote on 11/01/2016 10:57:30 PM: > > > > > > On Mon, Jan 11, 2016 at 5:05 AM, Liran Schour <lir...@il.ibm.com> wrote: > > > Andy Zhou <az...@ovn.org> wrote on 09/01/2016 02:02:32 AM: > > > > > > > > On Fri, Jan 8, 2016 at 8:58 AM, Liran Schour <lir...@il.ibm.com> > > wrote: > > > > Andy Zhou <az...@ovn.org> 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. > > I don't think Andy is asking to implement such a change. I think he's > asking to write the spec so that it is more amenable to extension in > that direction in the future. That seems reasonable to me. > > > 2. Do we want to allow "monitor_cond_change" to change the json-value > > associated with the monitor session? > > I think what Andy is seeking to accomplish here is some way for the > client to know when it has received an accurate snapshot of the database > given the new condition. Changing the json-value is an effective way to > do that, *if* the condition actually caused any rows to appear or > disappear; if it didn't, though, there's no way for the client to know. > Maybe we need to mandate that the server sends at least one update when > the condition changes, even if it's an empty update. > OK, will submit soon the next version of patches according to what we discussed. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
