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>. > > 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. > > > > I am not sure I understand your intention. Do you want to be able to > change the monitor_id due to a "monitor_update"? What is the use case for > that? > 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. 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? > > > 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? > > > Noticed that tabs are used in some files, for example, condition.[ch] > > > > Will fix that in the next patch series. > > > 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. > > > > Will use static column and data values for boolean functions. > > > 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. > > > > This is a typo. Will fix that in the next version. > > >> > >> 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 > jsoncache that > >> will be matched against condition clauses in the future. > >> > > > > Can we add conditions as part of cache selector? > > > > The possible option that I thought about is creating a cache per row in > case that we have a non-empty conditions and to add the specific condition > clause that where true matched in the conditions evaluation. We should > keep the whole db cache for empty conditions cases. I think that we can > add this cache functionality later. Sure, we can work on extending the cache model later. > > Thanks for the review, will send a new patch series with the > modifications. > Looking forward to them! Thanks for working on this feature! > > - Liran > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
