So, for the vote of using the validation dependencies we had 5 +1's for using the Validation libs
So the vote passes, that these dependencies will be used for the Golang Proxy validation rules. This PR will be can now be merged: https://github.com/apache/incubator-trafficcontrol/pull/1766 Thank you for taking the time to respond, -Dew On Wed, Jan 17, 2018 at 9:25 AM, Volz, Dylan <[email protected]> wrote: > +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 > >>>>>>>> > >>>>> > >> > >> > > > >
