rawlinp commented on a change in pull request #5144: URL: https://github.com/apache/trafficcontrol/pull/5144#discussion_r504792069
########## File path: CHANGELOG.md ########## @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Traffic Ops: Added validation to prohibit assigning caches to topology-based delivery services - Traffic Ops: Added validation to prohibit removing a capability from a server if no other server in the same cachegroup can satisfy the required capabilities of the delivery services assigned to it via topologies. - Traffic Ops: Added validation to ensure that at least one server per cachegroup in a delivery service's topology has the delivery service's required capabilities. + - Traffic Ops: Added validation to ensure that at least one server exists in each cachegroup. Review comment: "... that that is used in a Topology" ########## File path: traffic_ops/traffic_ops_golang/topology/topologies.go ########## @@ -563,6 +614,23 @@ JOIN topology_cachegroup tc on t.name = tc.topology return query } +func selectEmptyCacheGroupsQuery() string { + // language=SQL + query := ` + SELECT c."name", COUNT(( + SELECT s2.id FROM server s2 + JOIN "type" t ON s."type" = t.id + WHERE s2.id = s.id + )) server_count + FROM cachegroup c + LEFT JOIN "server" s ON c.id = s.cachegroup + WHERE c."name" = ANY(CAST(:cachegroup_names AS TEXT[])) + GROUP BY c."name" + ORDER BY server_count Review comment: Can this simply be ``` SELECT c."name", COUNT(*) AS server_count FROM cachegroup c LEFT JOIN "server" s ON c.id = s.cachegroup WHERE c."name" = ANY(CAST(:cachegroup_names AS TEXT[])) GROUP BY c."name" ORDER BY server_count; ``` instead? ########## File path: traffic_ops/traffic_ops_golang/topology/topologies.go ########## @@ -146,13 +149,61 @@ func (topology *TOTopology) Validate() error { return util.JoinErrs(tovalidate.ToErrors(rules)) } +func (topology *TOTopology) checkForEmptyCacheGroups() error { + var ( + cacheGroupNames = make([]string, len(topology.Nodes)) + baseError = errors.New("unable to check for cachegroups with no servers") + systemError = "checking for cachegroups with no servers: %s" + parameters = map[string]interface{}{} + query = selectEmptyCacheGroupsQuery() + ) + for index, node := range topology.Nodes { + cacheGroupNames[index] = node.Cachegroup + } + parameters["cachegroup_names"] = pq.Array(cacheGroupNames) + + rows, err := topology.ReqInfo.Tx.NamedQuery(query, parameters) + if err != nil { + log.Errorf(systemError, err.Error()) + return baseError + } + + var ( + serverCount int + cacheGroup string + cacheGroups []string + ) + defer log.Close(rows, "unable to close DB connection when checking for cachegroups with no servers") + for rows.Next() { + if err := rows.Scan(&cacheGroup, &serverCount); err != nil { + log.Errorf(systemError, err.Error()) + return baseError + } + if serverCount != 0 { + break + } + cacheGroups = append(cacheGroups, cacheGroup) + } + + if cacheGroups != nil { Review comment: nit: might read better as `len(cacheGroups) > 0` ---------------------------------------------------------------- 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: [email protected]
