I see, just looked at SliceScan, I mistakenly assumed it was how they implement selecting into a slice of structs which is one of the features that is very useful versus the straight sql implementation.
On 9/13/17, 9:59 AM, "Chris Lemmons" <[email protected]> wrote: That's not the problem that SliceScan helps with. SliceScan helps when you don't have control of the sql statement and you don't know the columns that will be returned, or how many will be returned. MapScan does the same, but puts it into a map, so you can iterate over them. That's a major feature that isn't available in the standard `sql` library, and would definitely justify the inclusion of `sqlx`. I don't think that's something we really want, though, because we control the sql statements. On Wed, Sep 13, 2017 at 8:54 AM, Volz, Dylan (Contractor) < [email protected]> wrote: > Yes Select, or the SliceScan is one of the reasons I would use sqlx, I am > pulling out sets using IN and left outer joins to avoid the tight looping > issues in some endpoints in the perl. > > On 9/13/17, 8:42 AM, "Chris Lemmons" <[email protected]> wrote: > > 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 <[email protected] > > > 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 < > [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 > > > > > > > > > > > > > > > > > > > >
