Hi
[...]
>> I miss also the relation between oprators and between the content
>> of operators. I mean AND or OR. How I understand your example:
>>
>> + local filter = {
>> + lt={{"gpc0", 1}, {"gpc1", 2}},
>> + gt={{"conn_rate", 3}},
>> + eq={{"conn_cur", 4}}
>> + }
>>
>> Are you sure that the syntax <operator>=<list of left, right operands>
>> is a good format ? Maybe something like the following, with the operator
>> as argument between the two operands. lines are implicitly OR, and columns
>> are AND:
>>
>> + local filter = {
>> + {{"gpc0", "lt", 1}, {"gpc1", "lt", 2}},
>> + {{"conn_rate", "gt", 3}},
>> + {{"conn_cur", "eq", 4}}
>> + }
> Actually, I was playing with some other ideas, and it was useful to be
> able to "preselect" filter operators.
> However, the CLI doesn't even support more than one, maybe we don't need
> to complicate too much. Maybe we can simplify to this:
>
> local filter = {
> {"gpc0", "lt", 1},
> {"gpc1", "lt", 2},
> {"conn_rate", "gt", 3},
> {"conn_cur", "eq", 4}
> }
>
> The default operator would be AND, and we would not support other
> operators (to keep the things simple). e.g. example use case for the
> filter would be to filter out on gpc0 > X AND gpc1 > Y
>
> If this sounds good, I can update/simplify the code.
Ok, it sounds good. I think this kind of syntax is easily understandable
and it allow a good way for filtering values.
>> Idea of extension for the future: Maybe it will be safe to compile
>> sticktable filter during the initialisation of the Lua code, to avoid
>> runtime errors ?
> I'm returning runtime errors since it can be easy to mix up data from
> the client side (most probably data would come as json table, then
> transformed to Lua table)
ok
[...]
>> Line 274 of your patch, I don't see any HA_SPIN_LOCK(STK_TABLE_LOCK
>> I don't known very well the thread, so maybe there are useles, maybe no.
> hlua_stktable_lookup() uses stktable_lookup_key() which does have locks,
> so I guess that it should be fine then?
sure !
[...]
>> l.365, 369: The user doesn't have context about the error. there are the
>> first entry of the table, the second ? Which operator doesn't exists ?
>>
>> L.380, 384: Which line is wrong ?
> Yes, it is somwehat cryptic. I've tried to avoid returning user supplied
> data in the error messages. We can revisit this if/when we change the
> filter table format.
ok
>> L.431: Your release the lock, so the next element relative to the current
>> "n", can disappear and the ebmb_next() can return wrong memory.
> I was under impression that we only have to acquire lock and increment
> ref_cnt (so we can be sure our current node n is not deleted)
> ebmb_next() is called only when we're holding lock, first and every
> other iteration, i.e.
>
> HA_SPIN_LOCK(STK_TABLE_LOCK, &t->lock);
> eb = ebmb_first(&t->keys);
> for (n = eb; n; n = ebmb_next(n)) {
> ...
> ts->ref_cnt++;
> HA_SPIN_UNLOCK(STK_TABLE_LOCK, &t->lock);
> ...
>
> HA_SPIN_LOCK(STK_TABLE_LOCK, &t->lock);
> }
>
> Or I didn't get your point?
ok, you probably right.
Thierry
