Ben Pfaff <[email protected]> wrote on 25/02/2016 02:25:32 AM:
> On Thu, Feb 18, 2016 at 11:46:40AM +0000, Liran Schour wrote:
> > Change ovsdb_condition to be a 3-element json array or a boolean
value.
> > Conditions utilities will be used later for conditional monitoring.
> >
> > Signed-off-by: Liran Schour <[email protected]>
> >
> > ---
> > v3->v4:
> > * Pass index_map only to ovsdb_condition_evaluate_or_datum()
> > * Added ovsdb_condition_is_[true|false]()
> > * Added ovsdb_condition_get_max_function()
> > * Added ovsdb_condition_diff()
> >
> > v2->v3:
> > * Remove condition_add() and condition_remove() and all sub-functions
> > * Allow single bollean value XOR <condition>
>
> Thanks for working on this.
>
> The naming here starts to be confusing. The difference between
> ovsdb_condition_evaluate() and ovsdb_condition_evaluate_or_datum() is
> not obvious. I guess that the former is for determining whether every
> clause is true and the other whether any clause is true; names like
> ovsdb_condition_is_every_clause_true() and
> ovsdb_condition_is_any_clause_true() would be easier to understand.
>
Will change the names according to your suggestion.
> It's totally unclear why the latter needs an extra index_map parameter
> or what goes in it. I think that a comment needs to explain that too.
>
Will add a comment to explain the usage of index_map.
> A lot of the function declarations here violate the coding style.
> Instead of:
> void ovsdb_condition_init(struct ovsdb_condition *cnd)
> please write:
> void
> ovsdb_condition_init(struct ovsdb_condition *cnd)
>
Will Fix in the next series.
> The new "functions" violate the comment right above them:
> /* These list is ordered in ascending order of the fraction of
> tables row that
> * they are (heuristically) expected to leave in query results. */
> #define OVSDB_FUNCTIONS \
> + OVSDB_FUNCTION(OVSDB_F_FALSE, "false") \
> + OVSDB_FUNCTION(OVSDB_F_TRUE, "true") \
>
I will update this comment.
> It seems strange that ovsdb_condition_evaluate_or_datum() returns true
> if there are no clauses. What's the rationale?
>
The rationale here is that we want that an empty condition will be treated
as implicit true in conditional monitoring. In that way if the user send a
monitor_cond request with an empty "where" condition, The result will be
the same as the former monitor2 request.
Maybe ovsdb_condition_evaluate_or_datum() should return true only if one
of the clauses is true and check if condition is empty before calling
ovsdb_condition_evaluate_or_datum(). What do you think?
> I'm not comfortable with tests like this:
> + if (clause->function > OVSDB_F_TRUE) {
> and this:
> + if (c->function <= OVSDB_F_TRUE) {
> because they break unexpectedly if someone reorders the enumeration.
>
Will change it to explicit check.
> This test is a really bad idea because n_clauses is unsigned:
> + if (a->n_clauses != b->n_clauses) {
> + return a->n_clauses - b->n_clauses;
> + }
> I think that bug demonstrates that there need to be some more tests.
>
Will fix this and add test.
> ovsdb_condition_cmp() should probably be named
> ovsdb_condition_cmp_3way() to match the convention elsewhere.
>
Fixed.
> ovsdb_conditon_diff() misspells the word "condition".
>
Right.
> Naming in diff functions can sometimes be tricky. The
> ovsdb_conditon_diff() parameters might be easier to understand if they
> were named a_only and b_only, that is, a_only received the clauses that
> were only in a and b_only received the clauses that were only in b.
>
Will change that.
> ovsdb_condition_is_true() assumes the condition is evaluated in one
> particular way (I guess by ovsdb_condition_evaluate_or_datum()). This
> should probably be documented in a comment, at least.
>
Right.
> I don't understand the purpose of ovsdb_condition_max_function().
>
I needed a way to tell if a condition has only functions of type
{false,true,"==","includes}. I need that to start track changes according
to condition's clauses to reduce computation in conditional monitoring on
the server side. Maybe I should replace it to a more explicit function
like: ovsdb_condition_is_every_function_false_or_equal(). What do you
think?
Thanks for the review. I am planning to submit a new version of the series
(V5) with bug fixes of the clause based change tracking implementation
soon.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
