I see that there's a significant preference to not list the members of the struct in the Scan function. That works.
I'm still not certain I understand why we'd prefer to bring in all of `sqlx` instead of writing a single, fairly simple function to return the columns for the scan. Are there plans to use MapScan or SliceScan? Cause that's really where `sqlx` starts to be worth it's cost. Still, to be sure we're all on the same page wrt performance, the reason the performance is fairly good is because the cost is per-query, not per-row. Since we're pretty good about our queries, I'm fairly happy with it's performance. If we're doing queries in a tight loop, it will start to bite us, but network delay would bite us harder. On Wed, Sep 13, 2017 at 8:18 AM, Dewayne Richardson <dewr...@gmail.com> wrote: > I also agree but I think your argument about future cost is more > "speculative" than reality. If we're smart about how we integrate sqlx, > then we can hedge the bet. > > -Dew > > On Tue, Sep 12, 2017 at 7:52 PM, Robert Butts <robert.o.bu...@gmail.com> > 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) < > > dylan_v...@comcast.com> 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" <derek_geli...@comcast.com> > 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 <dewr...@gmail.com > > > <mailto:dewr...@gmail.com>> 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 > > > > > > > > > > > >
