I’m +1 as I didn’t state it in the last email.
As far as sqlx being embedded in every endpoint and query if we remove it, I
disagree, it will only require changes anywhere we use StructScan or another
sqlx specific feature; for pulling out a few fields into variables for use for
example, doing execs, deletes, and other queries the syntax and code being
called is exactly the same.
On 9/12/17, 9:43 PM, "Jan van Doorn" <j...@knutsel.com> wrote:
I’m +1 on using sqlx. We’ve done this dance, what 3 times now already?
Let’s just use it and move on.
Just my $0.02.
Rgds,
JvD
> On 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
>>>>
>>>>
>>>>
>>
