I’m +1 on using sqlx. We’ve done this dance, what 3 times now already? Let’s just use it and move on.
Just my $0.02. Rgds, JvD > On Sep 12, 2017, at 9:08 PM, Chris Lemmons <[email protected]> wrote: > > @dan, how do you feel about the middle-ground I proposed: a > reflection-based function that would look like this: > rows.Scan(FieldsOf(&server)...) ? > > On Tue, Sep 12, 2017 at 9:05 PM, Dan Kirkwood <[email protected]> wrote: > >> As one ready to jump in and add more endpoints, I'm a strong +1 on >> using sqlx. I agree that adding a new dependency should not be done >> without consideration, but I find the sqlx version much more readable >> and easier to approach than either your or dew's version of non-sqlx >> and would be much easier to approach for one unfamiliar with details >> of this project. For me, it's worth it. >> >> strong +1 >> >> -dan >> >> >> On Tue, Sep 12, 2017 at 7:52 PM, Robert Butts <[email protected]> >> wrote: >>> I am a pretty big -1 on sqlx. Those PRs are extremely deceptive. >>> >>> Those lines are entirely unnecessary. >>> >>> I have created an example PR at https://github.com/apache/incu >>> bator-trafficcontrol/pull/1165 >>> >>> The relevant commits are >>> https://github.com/apache/incubator-trafficcontrol/pull/1165 >>> /commits/6fc735d7f97eaaffbf08e8457b7ccb6bf14baca0 >>> https://github.com/apache/incubator-trafficcontrol/pull/1165 >>> /commits/6939ee1d401c571af139db53b018a5e53f80c02a#diff-219ca >>> ea1a282285fe1cc21e53bf9dafbL26 >>> >>> As you can see, the only difference is that `rows.StructScan(&s)` >> becomes ` >>> rows.Scan(&s.Cachegroup, &s.CachegroupId, &s.CdnId, &s.CdnName, &s. >>> DomainName, &s.Guid, &s.HostName, &s.HttpsPort, &s.Id, &s.IloIpAddress, >> &s. >>> IloIpGateway, &s.IloIpNetmask, &s.IloPassword, &s.IloUsername, &s. >>> InterfaceMtu, &s.InterfaceName, &s.Ip6Address, &s.Ip6Gateway, >> &s.IpAddress, >>> &s.IpGateway, &s.IpNetmask, &s.LastUpdated, &s.MgmtIpAddress, &s. >>> MgmtIpGateway, &s.MgmtIpNetmask, &s.OfflineReason, &s.PhysLocation, &s. >>> PhysLocationId, &s.Profile, &s.ProfileDesc, &s.ProfileId, &s.Rack, &s. >>> RevalPending, &s.RouterHostName, &s.RouterPortName, &s.Status, >> &s.StatusId, >>> &s.TcpPort, &s.ServerType, &s.ServerTypeId, &s.UpdPending, &s.XmppId, &s. >>> XmppPasswd)` >>> >>> It is a one-line difference per endpoint, not 100 lines. (Plus column >>> annotations on every struct field for sqlx) >>> >>> That said, I agree the former is better for readability. The issue is the >>> maintenance cost, when-not-if sqlx stops being maintained. It will be >>> embedded in Traffic Ops, in every single endpoint and query. We'll be in >>> exactly the same position we are with Goose, stuck with an unmaintained >> and >>> probably vulnerable library, which is very expensive in developer-hours >> to >>> remove. Surely most of us here have been in this situation, with legacy >>> unmaintained apps, libraries, compilers, etc? >>> >>> By `cloc` Sqlx is 3400 lines, which doesn't sound like a lot, but a big >>> percentage of that is Go Reflection, which is exceedingly painful to >> write, >>> debug, and maintain. >>> >>> Is standard Go really that much more difficult to write? The above is one >>> of the worst cases (along with Deliveryservices), most of our tables >> aren't >>> nearly that big. It doesn't seem likely to cause bugs, any mismatches >>> should be immediately caught when running the first time, and certainly >> by >>> the tests we've been mandating. >>> >>> I'm not wholesale against third-party libraries. Often the benefit >>> outweighs the cost; for example, `sqlmock`, and in the future, `jwt`. But >>> in this particular case, the maintenance cost far outweighs the benefit. >>> >>> This isn't a black-and-white issue, it's a cost-benefit analysis. Sqlx is >>> marginally easier to write, for an unknowable and potentially enormous >>> future cost. >>> >>> >>> On Tue, Sep 12, 2017 at 6:54 PM, Volz, Dylan (Contractor) < >>> [email protected]> wrote: >>> >>>> It would be maintaining about a 1500 line codebase (excluding tests with >>>> ~70% coverage), it uses reflection and tag introspection so it isn’t the >>>> simplest go code but it does seem to be well commented. >>>> >>>> On 9/12/17, 6:36 PM, "Gelinas, Derek" <[email protected]> >> wrote: >>>> >>>> After looking at the code, and given the work I've been doing with >>>> rewriting the config file endpoints, I have to say sqlx all the way. >>>> What's involved in the maintenance? >>>> >>>> Derek >>>> >>>> On Sep 12, 2017, at 8:28 PM, Dewayne Richardson <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> There has been quite a bit of discussion about how to move forward >>>> with the >>>> Traffic Ops Rewrite in terms of Golang dependencies. Currently >> there >>>> is >>>> only one dependency for Mocking out the database for unit testing >>>> called >>>> https://github.com/DATA-DOG/go-sqlmock. Another that we want to >>>> evaluate is >>>> https://github.com/jmoiron/sqlx for accessing Postgres to help with >>>> minimizing Golang boilerplate code. The following are the Pros and >>>> Cons >>>> are listed to he >>>> lp decide (please add any that I failed to include) >>>> >>>> >>>> Pros >>>> - Developer productivity increases (less boilerplate code) >>>> - Less Developer errors through tedious field mapping >>>> - Active Development >>>> >>>> Cons >>>> - Another dependency to maintain if it is no longer supported >>>> >>>> Performance >>>> The performance penalty is neglible (I tested the /api/1.2/servers >>>> endpoint, very loosely, in the Comcast Open Stack lab using the >>>> same VM and Apache Bench with 1000, 10000, and 10000 separate >> requests >>>> and the performance was +/-5% depending on the cloud resources that >>>> were >>>> active). >>>> Remember, this endpoint is still 20x faster than the Traffic Ops >> Perl >>>> version. >>>> >>>> >>>> So, please review the following PR's specifically noting the >>>> servers.go and >>>> servers_test.go files (also browser around to see our progress) >>>> >>>> WITH Sqlx >>>> https://github.com/dewrich/incubator-trafficcontrol/blob/ >>>> 4a09d4188be9dd10fb524f2320aeae6fc44e45e3/traffic_ops/ >>>> traffic_ops_golang/servers.go >>>> https://github.com/dewrich/incubator-trafficcontrol/blob/ >>>> 4a09d4188be9dd10fb524f2320aeae6fc44e45e3/traffic_ops/ >>>> traffic_ops_golang/servers_test.go >>>> >>>> WITHOUT Sqlx >>>> https://github.com/dewrich/incubator-trafficcontrol/blob/ >>>> 89acc848c12f45361b42c4343364dcffe8a7773a/traffic_ops/ >>>> traffic_ops_golang/servers.go >>>> https://github.com/dewrich/incubator-trafficcontrol/blob/ >>>> 89acc848c12f45361b42c4343364dcffe8a7773a/traffic_ops/ >>>> traffic_ops_golang/servers_test.go >>>> >>>> >>>> This vote will be closed by noon this Friday 9/25/2017 >>>> >>>> Thanks, >>>> >>>> -Dew >>>> >>>> >>>> >>
