+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 > >>>>>> > >>> > >
