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. > > > > The goal is for the client to be able tell if an update message are > > generated before or after the condition changes. > > To be precise, the monitor_id I mentioned refers to <json-value> > > within the monitor message. > > > > Same as above I can implement this but maybe we should have a broader > agree about it. Sounds good. > > > > In addition, it may be good to explicitly define the timing > > requirements for monitor updates. Is it reasonable to require > > New updates be sent only after the monitor updates messages are acked? > > > > Will document it in the man page. > > > > 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. > > > > > > > Yes, any columns should be allowed in condition. > > Perfect. It will be nice to make this clear in the ovsdb-server man > page. > > > > > 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? > > > > > > > During the request for conditions change, A new change list is being > > recorded holding all the rows of the table with the changes accord since > > the last flush. This change list will be sent to the client on update > > notification and only then condition evaluation will take place. I do not > > see an inconsistency possibility here. Do you see one? > > Sorry, you are right, there is no race condition here. I missed > > that conditions are evaluated for > > each Jsonrpc connection separately. On the other hand, Would it be > > nice for monitors with the same > > conditions not have to evaluate the same conditions multiple times? > > > > After submitting the basic functionality I plan to implement a cache that > will be indexed by condition's clauses and will reduce the need to evaluate > conditions per row. > Sounds good. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
