+1
On 1/17/18, 5:43 AM, "John Rushford" <[email protected]> wrote:
+1
Sent from my iPad
> On Jan 16, 2018, at 2:07 PM, Dave Neuman <[email protected]> wrote:
>
> +1
>
>> On Tue, Jan 16, 2018 at 12:58 Jan van Doorn <[email protected]> wrote:
>>
>> +1 on using libs.
>>
>>> On Jan 16, 2018, at 10:52 AM, Dan Kirkwood <[email protected]> wrote:
>>>
>>> +1 -- agree with Jeff -- the validation of the fields of
>>> deliveryservice is something that is incomplete in the Perl
>>> traffic_ops.
>>>
>>> These libraries make for concise code to do the validation so it will
>>> be easier to extend without much extra code. It will not be called
>>> on every API function, but only once on each POST/PUT which are a
>>> small minority of calls. It also need not be used in every case.
>>> That, to me, makes the reflection argument much less of a concern.
>>>
>>> I would like to see it go in sooner than Friday, but won't argue that
>> point..
>>>
>>> -dan
>>>
>>>
>>> On Tue, Jan 16, 2018 at 10:10 AM, Dewayne Richardson <[email protected]>
>> wrote:
>>>> So, it's been a few days on this topic and I'd like to call a vote for
>> the
>>>> dependencies listed in this thread. Please vote +1 or -1 by Noon
>> Friday so
>>>> that we can move forward the Golang Proxy development.
>>>>
>>>> Thanks,
>>>>
>>>> -Dew
>>>>
>>>> On Thu, Jan 11, 2018 at 10:53 AM, Jeff Elsloo <[email protected]>
>> wrote:
>>>>
>>>>> I don't think we should assume anything about the performance just
>>>>> because it uses reflection. Yes, traditionally reflection is
>>>>> computationally expensive, however, when used properly the penalty can
>>>>> be negligible. I don't think we have enough understanding of these
>>>>> libraries to know whether there is a concerning performance penalty.
>>>>>
>>>>> As Dewayne said, create, update and delete actions represent a tiny
>>>>> fraction of the overall requests into TO. Given that the majority of
>>>>> these actions are performed by humans, I would be shocked if there was
>>>>> a perceptible performance difference with the reflection based
>>>>> validation in place. It's not like we're trying to validate enormous
>>>>> and complex objects here; we're talking 20 fields or so for any given
>>>>> post.
>>>>>
>>>>> I'm +1 on using validation libraries such as these even if they use
>>>>> reflection, provided that we do not see dramatic changes in
>>>>> performance. I think that's highly unlikely in this case.
>>>>> --
>>>>> Thanks,
>>>>> Jeff
>>>>>
>>>>>
>>>>> On Thu, Jan 11, 2018 at 10:07 AM, Chris Lemmons <[email protected]>
>>>>> wrote:
>>>>>> True, but how many of those out-of-the-box checks are both useful and
>>>>>> relevantly complex?
>>>>>>
>>>>>> To me, the cool part of ozzo is the way it collects the output and
>>>>>> formats it. That's unfortunately also the computationally expensive
>>>>>> part.
>>>>>>
>>>>>> On Thu, Jan 11, 2018 at 9:49 AM, Dewayne Richardson <
>> [email protected]>
>>>>> wrote:
>>>>>>> Please keep in mind that we do not Create/Update/Delete very often
in
>>>>>>> Traffic Ops, so the performance penalty for Validation should be
>> taken
>>>>> into
>>>>>>> consideration. I also don't want to re-invent all of those
>>>>> out-of-the-box
>>>>>>> field level checks by hand when I can just use them from here:
>>>>>>> https://github.com/asaskevich/govalidator#list-of-functions
>>>>>>>
>>>>>>> -Dew
>>>>>>>
>>>>>>> On Thu, Jan 11, 2018 at 9:24 AM, Chris Lemmons <[email protected]>
>>>>> wrote:
>>>>>>>
>>>>>>>> I like the output style, but I'm a bit concerned on the performance
>>>>>>>> front. ozzo appears to do all it's magic with heavy use of
>> reflection,
>>>>>>>> which is often a slow spot in go. Most places, it wouldn't matter
>>>>>>>> much, but this will be called on every element of every API
>> function,
>>>>>>>> so a nod toward performance may be in order. Have we done some
>>>>>>>> measurement to see whether this adds a relevant amount of overhead
>> to
>>>>>>>> the calls? Or are the calls still dominated by the DB lookup?
>>>>>>>>
>>>>>>>> Relatedly, is this a major advantage over something like this:
>>>>>>>>
>>>>>>>> if ds.Active == nil { errMsgs = append(errMsgs, `"active" must be
>>>>>>>> provided`) }
>>>>>>>>
>>>>>>>> On Thu, Jan 11, 2018 at 8:49 AM, Dewayne Richardson <
>>>>> [email protected]>
>>>>>>>> wrote:
>>>>>>>>> We've been moving along with more functionality in the Golang
>> proxy,
>>>>>>>> mostly
>>>>>>>>> the Read's up until now, comparatively TO does much fewer
>>>>> Create/Updates.
>>>>>>>>> Our current task is to circle back and start implementing the
>>>>> (C)reate,
>>>>>>>>> (U)pdate, and (D)eletes. One of the obvious needs for the this
>> task
>>>>> are
>>>>>>>>> validation rules. I've been doing research to figure out the
>>>>> cleanest
>>>>>>>> and
>>>>>>>>> most maintainable way to rewrite the Perl validation rules in Go.
>>>>>>>>>
>>>>>>>>> TC Issue for tracking
>>>>>>>>> https://github.com/apache/incubator-trafficcontrol/issues/1756
>>>>>>>>>
>>>>>>>>> These are the two dependencies I'd like to leverage and provide
>>>>> feedback:
>>>>>>>>>
>>>>>>>>> Both are MIT Licenses
>>>>>>>>> Uses normal programming constructs rather than error-prone struct
>>>>> tags to
>>>>>>>>> specify how data should be validated.
>>>>>>>>> https://github.com/go-ozzo/ozzo-validation
>>>>>>>>> https://github.com/go-ozzo/ozzo-validation/blob/master/LICENSE
>>>>>>>>>
>>>>>>>>> Core Validation library that the prior library uses that has a lot
>> of
>>>>>>>>> useful convenience methods that I'd rather not re-invent
>>>>>>>>> https://github.com/asaskevich/govalidator
>>>>>>>>> https://github.com/asaskevich/govalidator#list-of-functions
>>>>>>>>> https://github.com/asaskevich/govalidator/blob/master/LICENSE
>>>>>>>>>
>>>>>>>>> And here is how I've used these as sample validation rules that
>> I've
>>>>>>>>> implemented as a POC:
>>>>>>>>>
>>>>>>>>> https://github.com/dewrich/incubator-trafficcontrol/blob/
>>>>>>>> tor-api-ds/traffic_ops/traffic_ops_golang/deliveryservice/
>>>>>>>> deliveryservices.go#L93
>>>>>>>>>
>>>>>>>>> Existing Mojo Perl Rules for comparison.
>>>>>>>>> https://github.com/apache/incubator-trafficcontrol/blob/
>>>>>>>> master/traffic_ops/app/lib/API/Deliveryservice.pm#L1363
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -Dew
>>>>>>>>
>>>>>
>>
>>
