On 12 Jan 2022, at 16:46, Adrian Moreno wrote:
> On 12/24/21 15:07, Eelco Chaudron wrote:
>>>>> +class OFPFlowFactory:
>>
>> See my comments on patch 8, where I do think we should get rid of this
>> class, and update the OFPFlow class to take this string at init.
>> Being more OOO it would look like this:
>>
>> def __init__(self, ofp_string, id=None):
>> """Constructor"""
>> sections = self._sections_from_string(ofp_string)
>> super(OFPFlow, self).__init__(sections, ofp_string, id)
>>
>> Where sections_from_string(ofp_string) is just the same code as in this
>> class but returns the sections.
>>
>> @staticmethod
>> def _sections_from_string(ofp_string):
>> ...
>>
>
> That was more or less how I originally implemented it. I then refactored it
> into a factory class because when parsing thousands of flows I saw a
> considerable amount of time being spent on parser "preparation", this is:
> creating all the KVParser objects. So I created the factory just to cache
> this data.
>
> Another alternative would be to make them global (for the class) and
> initialize them the first time we parse a flow so we could keep the nicer
> syntax. I'm trying to avoid running functions on module load. WDYT?
Yes making use of static class variables as you suggest in patch 8 would do the
job.
Thanks for following up on all my comments :)
>>>>
>>>> Do we need OFPFlowFactory(object):, some times flake reports this as H238.
>>>> But might no longer be valid with the latest python3.
>>>
>>> Not 100% sure either, I'll check. Thanks.
>>>
>>>>
>>>>> + """OpenFlow Flow Factory is a class capable of creating OFPFLow
>>>>> objects"""
>>>>> +
>>>>> + def __init__(self):
>>>>> + self.info_decoders = self._info_decoders()
>>>>> + self.match_decoders = KVDecoders(
>>>>
>>>> The name suggests that we return KVDecoders() as the __info_decoders()
>>>> does, I think all three should return the same.
>>>>
>>>
>>> What name exactly? Do you mean the _flow_match_decoders and _field_decoders?
>>> Or do you mean match_decoders should be a function rather than the object
>>> directly?
>>
>> Trying to remember what my thoughts were ;) Guess I wondered why
>> _flow_match_decoders() was not doing the ‘return
>> KVDecoders({**self._field_decoders(), **self._flow_match_decoders()})’.
>>
>
> I'll unify the names to make them more consistent.
>
> [...]
>
>>>>> +
>>>>> + @classmethod
>>>>> + def _output_actions_decoders(cls):
>>>>> + """Returns the decoders for the output actions"""
>>>>
>>>> If you have them, I think it might be useful, to add some example strings
>>>> to the individual decoders. This way, it's easy to see what they intend to
>>>> decode.
>>>>
>
> Ack.
>
> [...]
>>>
>>>>> +def decode_learn(action_decoders):
>>>>
>>>> It’s getting late, and I have a hard time (focussing ;) understanding
>>>> where the value for this one comes from? I'll pick it up from here when I
>>>> continue the review.
>>>>
>>>
>>> I hope I can clarify. Learn action has two added complexities:
>>>
>>> - It accepts any other action key-value. For this we need to create a
>>> wrapper around the pre-calculated action_decoders.
>>>
>>> - The way fields can be specified is augmented. Not only we have
>>> 'field=value', but we also have 'field=_src_' (where _src_ is another field
>>> name) and just 'field'. For this we need to create a wrapper of
>>> field_decoders that, for each "field=X" key-value we check if X is a
>>> field_name or if it's acually a value that we need to send to the
>>> appropriate field_decoder to process.
>>
>> Ack thanks, it makes sense now.
>
>
> I'll add a comment to make it clearer.
>
>
>
> --
> Adrián Moreno
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
