The deadline of Friday 09-15 at noon has passed, sqlx as a TO Golang Dependency has been accepted. The vote tally is 6 Aye to 2 Nay's
Have a good weekend, -Dew Results: Dan Kirkwood: +1 Derek Gelina: +1 Dewayne Richardson: +1 Dylan Volz: +1 Jan Van Doorn: +1 Jeremy Mitchell: +1 Chris Lemmons: -1 Rob Butts: -1 On Wed, Sep 13, 2017 at 10:05 AM, Volz, Dylan (Contractor) < [email protected]> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
