I'm also not 100% on sqlx 100% of the time, but what I'm 100% on is ease of use, development productivity, and minimizing bug introduction.
-Dew On Tue, Sep 12, 2017 at 9:08 PM, Chris Lemmons <alfic...@gmail.com> 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 <dang...@gmail.com> 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 <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 > > >> > > >> > > >> > > >
