You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficcontrol.apache.org by Dewayne Richardson <de...@gmail.com> on 2017/09/13 00:27:55 UTC
Traffic Ops Rewrite Golang Dependency - sqlx
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
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Dewayne Richardson <de...@gmail.com>.
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 <al...@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 <da...@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 <ro...@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_Volz@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" <De...@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 <
> dewrich@gmail.com
> > >> <ma...@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
> > >>
> > >>
> > >>
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by "Volz, Dylan (Contractor)" <Dy...@comcast.com>.
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" <jv...@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 <al...@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 <da...@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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
>>>> <ma...@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
>>>>
>>>>
>>>>
>>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Jan van Doorn <jv...@knutsel.com>.
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 <al...@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 <da...@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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
>>>> <ma...@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
>>>>
>>>>
>>>>
>>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Chris Lemmons <al...@gmail.com>.
@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 <da...@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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> >> <ma...@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
> >>
> >>
> >>
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Dan Kirkwood <da...@gmail.com>.
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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
>> <ma...@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
>>
>>
>>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Dewayne Richardson <de...@gmail.com>.
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) <
Dylan_Volz@comcast.com> 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" <al...@gmail.com> 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) <
> Dylan_Volz@comcast.com> 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" <al...@gmail.com> 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 <
> dewrich@gmail.com
> > >
> > 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 <
> > robert.o.butts@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_Volz@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_Gelinas@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 <
> > dewrich@gmail.com
> > > > > <ma...@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
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
>
>
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by "Volz, Dylan (Contractor)" <Dy...@comcast.com>.
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" <al...@gmail.com> 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) <
Dylan_Volz@comcast.com> 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" <al...@gmail.com> 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 <dewrich@gmail.com
> >
> 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 <
> robert.o.butts@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_Volz@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_Gelinas@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 <
> dewrich@gmail.com
> > > > <ma...@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
> > > >
> > > >
> > > >
> > >
> >
>
>
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Chris Lemmons <al...@gmail.com>.
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) <
Dylan_Volz@comcast.com> 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" <al...@gmail.com> 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 <dewrich@gmail.com
> >
> 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 <
> robert.o.butts@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_Volz@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_Gelinas@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 <
> dewrich@gmail.com
> > > > <ma...@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
> > > >
> > > >
> > > >
> > >
> >
>
>
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by "Volz, Dylan (Contractor)" <Dy...@comcast.com>.
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" <al...@gmail.com> 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 <de...@gmail.com>
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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> > > <ma...@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
> > >
> > >
> > >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Chris Lemmons <al...@gmail.com>.
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 <de...@gmail.com>
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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> > > <ma...@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
> > >
> > >
> > >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Dewayne Richardson <de...@gmail.com>.
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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> > <ma...@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
> >
> >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Dewayne Richardson <de...@gmail.com>.
I think we're not looking at this correctly, if we look at using "sqlx" on
a case by case basis, especially if we plan on doing Microservices down the
road (where each MS could be coded differently depending on performance
needs), then it's not an "all or nothing" proposition. I'm just trying to
solve the 80/20 rule.
-Dew
On Tue, Sep 12, 2017 at 9:02 PM, Chris Lemmons <al...@gmail.com> wrote:
> > Why the fear of using dependencies?
>
> There's no fear of using dependencies. There's a recognition that
> dependencies aren't free and a balancing of the costs they entail. In this
> case, looking at the costs associate with `sqlx`, I don't think the
> benefits justify it. The way we're using it, we could write a single
> function to return the relevant interfaces. The scan call would look
> something like this: rows.Scan(FieldsOf(&server)...)
>
> I am 100% in favour of things that improve developer productivity, but we
> have to consider both the short and long term in that. That includes the
> developer time that will be spent tracking down `sqlx` issues (and check
> the issues page, there are several deeply mysterious bugs marked "could not
> reproduce"), verifying new updates, keeping abreast of security and
> critical issues and such. I think that, all things considered, the team
> will be more productive without `sqlx` than they would be with it.
>
> `sqlx` isn't a framework like Mojolicious or Gorilla Mux. It's syntactic
> sugar via reflection on sql calls.
>
> There's no plan to reinvent everything. There's a middle-ground between
> Not-Invented-Here and Leftpad-syndrome. It's just a cost-benefit analysis.
>
> On Tue, Sep 12, 2017 at 8:41 PM, Jeremy Mitchell <mi...@gmail.com>
> wrote:
>
> > @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 <al...@gmail.com>
> 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 <
> robert.o.butts@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_Volz@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" <De...@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 <
> > dewrich@gmail.com
> > > > > <ma...@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
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Chris Lemmons <al...@gmail.com>.
> Why the fear of using dependencies?
There's no fear of using dependencies. There's a recognition that
dependencies aren't free and a balancing of the costs they entail. In this
case, looking at the costs associate with `sqlx`, I don't think the
benefits justify it. The way we're using it, we could write a single
function to return the relevant interfaces. The scan call would look
something like this: rows.Scan(FieldsOf(&server)...)
I am 100% in favour of things that improve developer productivity, but we
have to consider both the short and long term in that. That includes the
developer time that will be spent tracking down `sqlx` issues (and check
the issues page, there are several deeply mysterious bugs marked "could not
reproduce"), verifying new updates, keeping abreast of security and
critical issues and such. I think that, all things considered, the team
will be more productive without `sqlx` than they would be with it.
`sqlx` isn't a framework like Mojolicious or Gorilla Mux. It's syntactic
sugar via reflection on sql calls.
There's no plan to reinvent everything. There's a middle-ground between
Not-Invented-Here and Leftpad-syndrome. It's just a cost-benefit analysis.
On Tue, Sep 12, 2017 at 8:41 PM, Jeremy Mitchell <mi...@gmail.com>
wrote:
> @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 <al...@gmail.com> 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 <ro...@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_Volz@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" <De...@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 <
> dewrich@gmail.com
> > > > <ma...@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
> > > >
> > > >
> > > >
> > >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Jeremy Mitchell <mi...@gmail.com>.
@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 <al...@gmail.com> 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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> > > <ma...@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
> > >
> > >
> > >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Chris Lemmons <al...@gmail.com>.
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 <ro...@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_Volz@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" <De...@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 <dewrich@gmail.com
> > <ma...@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
> >
> >
> >
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by Robert Butts <ro...@gmail.com>.
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_Volz@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" <De...@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 <dewrich@gmail.com
> <ma...@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
>
>
>
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by "Volz, Dylan (Contractor)" <Dy...@comcast.com>.
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" <De...@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 <de...@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
Re: Traffic Ops Rewrite Golang Dependency - sqlx
Posted by "Gelinas, Derek" <De...@comcast.com>.
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 <de...@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