@dewayne - you said "performance was +/-5% depending on the cloud resources that were active". How many milliseconds difference (average) are we talking about for ONE call to /api/1.2/servers (using sqlx and not using sqlx)? I'm assuming it is so small that we can rule out performance as a factor when considering sqlx vs. no sqlx.
As someone who has to write a lot of the API code, I'm a big fan of anything that increases developer productivity and as someone that has to review a lot of API PR's, I'm a big fan of anything that improves readability (and decreases the amount of lines of code I have to review). Productivity + readability == my idea of a good time. I'm confused. Why the fear of using dependencies? Do we plan to reinvent the wheel for everything in this Golang API rewrite (routing, logging, authentication, etc, etc) for fear that a 3rd-party library will stop being maintained? If it were me, I would have installed Gorilla Mux (or something similar) a long time ago, so we could get down to business of writing API endpoints and not have to deal in the minutia (and all the inevitable bug fixing) that comes with home-growing an API framework. Also, remember that homegrown frameworks have zero resources on the internet. Say what you will about Mojolicious but at least you could do some Googling to figure out how to use it. +1 on sqlx jeremy On Tue, Sep 12, 2017 at 8:37 PM, Chris Lemmons <[email protected]> wrote: > I'm also -1 on sqlx. > > That said, the "single line" in the `sql` version is 674 characters long. I > think we can agree that it's one heck of a line. > > We're focusing on the Scan call vs. the ScanStruct call, which I think is > the right place to look. Here's what `sql` requires: > - One identifier per column in the sql statement, ordered correctly, in the > Scan call. > > Here's what `sqlx` requires: > - One identifier per column in the sql statement, stored in the struct tag. > > In exchange for moving the identifiers from the struct tag to the scan > call, you lose a bit of performance and add a third-party dependency. > `sqlx` is moderately stable and moderately active, but both of those things > can change fairly quickly. Dependencies are great, when they're worth the > cost. > > We'd also be pulling in a fair bit of sqlx for what amounts to a single > function. That's somewhat excessive, I think. We don't need most of what it > offers. > > The second question is how much will using straight `sql` increase > maintenance, and likewise with `sqlx`. For dependencies, we need to keep > them up-to-date with bugfixes and security patches. We're not experts in > `sqlx`, so bugs in that library will be comparatively expensive to trace > and locate, if they exist. For the `sql` approach, the primary risk is just > that the columns wind up out of order. The unit tests as they stand will > prevent this. > > If we're really trying to skip having to type all the member variables in > order... there's an easier way to do that anyway. We've already got a > function returning the column names via reflection, we could just as easily > do the exact same thing with their pointers and pass that straight to Scan. > Which is the "best" of both worlds: you don't have to type the structs out > and you don't have to store them in the struct tag. That said... I can't > strongly recommend that technique because it has the same (though > technically lesser) performance penalty as `sqlx`, and it's still pretty > ugly. But it's better than pulling in an entire library for the benefit of > one fairly reasonably written function. > > I guess the bottom line is that I don't think listing out the columns is > that problematic. And even if it is, there are better solutions to that > problem than pulling in the full `sqlx` library. > > > 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 > > > > > > > > > > > >
