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? > 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? > 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. > 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? > 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. Thanks for the review, will send a new patch series with the modifications. - Liran _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
