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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
