+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 <dewr...@gmail.com> 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 <els...@apache.org> 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 <alfic...@gmail.com> >> 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 <dewr...@gmail.com> >> 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 <alfic...@gmail.com> >> 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 < >> dewr...@apache.org> >> >>> 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 >> >>> >>
