rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379176893
##########
File path: traffic_ops/traffic_ops_golang/crconfig/servers.go
##########
@@ -111,7 +111,7 @@ func getAllServers(cdn string, tx *sql.Tx)
(map[string]ServerUnion, error) {
// TODO select deliveryservices as array?
q := `
-select s.host_name, cg.name as cachegroup, concat(s.host_name, '.',
s.domain_name) as fqdn, s.xmpp_id as hashid, s.https_port, s.interface_name,
s.ip_address, s.ip6_address, s.tcp_port, p.name as profile_name,
cast(p.routing_disabled as int), st.name as status, t.name as type
+select s.host_name, cg.name as cachegroup, concat(s.host_name, '.',
s.domain_name) as fqdn, s.xmpp_id as hashid, s.https_port, s.interface_name,
case when s.ip_address_is_service = true then s.ip_address else '' end, case
when s.ip6_address_is_service = true then s.ip6_address else '' end,
s.tcp_port, p.name as profile_name, cast(p.routing_disabled as int), st.name as
status, t.name as type
Review comment:
How strongly do you feel about this, vs an `if` statement in Go?
The `case` here is a conditional. I know this feels like a nitpick, but in
my experience, embedding code in the database or queries leads to pain. I think
if we keep code as code, and data as data, and SQL as a declarative query, it
just makes everything easier to understand and work with, and less to
misunderstand or go wrong.
Here, the `case` is technically SQL, but it's an imperative `if` statement,
not declarative like SQL is intended to be. It's not a query, it's logic.
Would you object to changing this to something like:
```
qry := `
SELECT
s.host_name,
cg.name AS cachegroup,
CONCAT(s.host_name, '.', s.domain_name) AS fqdn,
s.xmpp_id AS hashid,
s.https_port,
s.interface_name,
s.ip_address_is_service,
s.ip6_address_is_service,
s.ip_address,
s.ip6_address,
s.tcp_port,
p.name AS profile_name,
CAST(p.routing_disabled AS int),
st.name AS status,
t.name AS type
FROM
server s
JOIN cachegroup cg ON cg.id = s.cachegroup
JOIN type t on t.id = s.type
JOIN profile p ON p.id = s.profile
JOIN status st ON st.id = s.status
WHERE
cdn_id = (SELECT id FROM cdn WHERE name = $1)
AND (st.name = 'REPORTED' or st.name = 'ONLINE' or st.name = 'ADMIN_DOWN')
`
rows, err := tx.Query(qry, cdn)
if err != nil {
return nil, errors.New("Error querying servers: " + err.Error())
}
defer rows.Close()
for rows.Next() {
port := sql.NullInt64{}
ip6 := sql.NullString{}
hashId := sql.NullString{}
httpsPort := sql.NullInt64{}
ipIsService := false
ipIsService := false
host := ""
status := ""
s := ServerUnion{}
if err := rows.Scan(&host, &s.CacheGroup, &s.Fqdn, &hashId,
&httpsPort, &s.InterfaceName, &ipIsService, &ip6IsService, &s.Ip, &ip6, &port,
&s.Profile, &s.RoutingDisabled, &status, &s.ServerType); err != nil {
return nil, errors.New("Error scanning server: " +
err.Error())
}
if !ipIsService {
s.Ip = util.StrPtr("")
}
if !ip6IsService {
s.Ip6 = util.StrPtr("")
}
```
Really, the very need for imperative logic here is a result of
denormalization. But we certainly don't have time to properly normalize our
schema right now.
I know my position is a little bit nuanced. If you feel strongly in the
other direction, I won't push for it. But if it's all the same to you, IMO the
separation of data vs code makes things easier and safer to work with.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
