You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/07/20 19:29:32 UTC
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4892: /cdns/capacity returns 500 error: capacity was zero
zrhoffman commented on a change in pull request #4892:
URL: https://github.com/apache/trafficcontrol/pull/4892#discussion_r457611698
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -117,10 +119,22 @@ func getMonitorsCapacity(tx *sql.Tx, monitors map[tc.CDNName][]string) (Capacity
}, nil
}
+func checkServiceInterface(tx *sql.Tx, name tc.InterfaceName, cacheName tc.CacheName) bool {
Review comment:
Since `checkServiceInterface` is used only once, it can be moved closer to the function that uses it.
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -166,21 +181,39 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
if !strings.HasPrefix(*cache.ServerType, string(tc.CacheTypeEdge)) {
continue
}
- if len(stats.KBPS) < 1 || len(stats.MaxKBPS) < 1 {
- continue
+ var kbps, maxKbps float64
+ for intName, intStats := range stats {
+ if len(intStats.KBPS) < 1 || len(intStats.MaxKBPS) < 1 {
+ continue
+ }
+ if checkServiceInterface(tx, intName, cacheName) {
+ if len(intStats.KBPS) >= 1 {
+ if kbps < intStats.KBPS[0].Value {
Review comment:
This can be a single *if* statement
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -166,21 +181,39 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
if !strings.HasPrefix(*cache.ServerType, string(tc.CacheTypeEdge)) {
continue
}
- if len(stats.KBPS) < 1 || len(stats.MaxKBPS) < 1 {
- continue
+ var kbps, maxKbps float64
+ for intName, intStats := range stats {
+ if len(intStats.KBPS) < 1 || len(intStats.MaxKBPS) < 1 {
+ continue
+ }
+ if checkServiceInterface(tx, intName, cacheName) {
+ if len(intStats.KBPS) >= 1 {
+ if kbps < intStats.KBPS[0].Value {
+ kbps = intStats.KBPS[0].Value
+ }
+ }
+ if len(intStats.MaxKBPS) >= 1 {
+ if maxKbps < intStats.MaxKBPS[0].Value {
Review comment:
This can also be a single *if* statement
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -117,10 +119,22 @@ func getMonitorsCapacity(tx *sql.Tx, monitors map[tc.CDNName][]string) (Capacity
}, nil
}
+func checkServiceInterface(tx *sql.Tx, name tc.InterfaceName, cacheName tc.CacheName) bool {
+ var result *bool
+ if err := tx.QueryRow(`SELECT i.service_address FROM ip_address AS i JOIN server as s on s.id = i.server WHERE i.interface=$1 AND s.host_name=$2`, name, cacheName).Scan(&result); err != nil {
+ log.Errorln("Error checking for service interface ", err)
+ return false
+ }
+ if *result == true {
+ return true
+ }
+ return false
Review comment:
Since we're returning `true` when `*result` is `true` and `false` otherwise, this can just be
```golang
return *result
```
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -117,10 +119,22 @@ func getMonitorsCapacity(tx *sql.Tx, monitors map[tc.CDNName][]string) (Capacity
}, nil
}
+func checkServiceInterface(tx *sql.Tx, name tc.InterfaceName, cacheName tc.CacheName) bool {
+ var result *bool
+ if err := tx.QueryRow(`SELECT i.service_address FROM ip_address AS i JOIN server as s on s.id = i.server WHERE i.interface=$1 AND s.host_name=$2`, name, cacheName).Scan(&result); err != nil {
Review comment:
Please reformat this query to have more line breaks for readability.
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -166,21 +181,39 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
if !strings.HasPrefix(*cache.ServerType, string(tc.CacheTypeEdge)) {
continue
}
- if len(stats.KBPS) < 1 || len(stats.MaxKBPS) < 1 {
- continue
+ var kbps, maxKbps float64
+ for intName, intStats := range stats {
+ if len(intStats.KBPS) < 1 || len(intStats.MaxKBPS) < 1 {
+ continue
+ }
+ if checkServiceInterface(tx, intName, cacheName) {
Review comment:
Can this *if* statement's be extracted to save a level of indentation?
Currently:
```golang
if condition {
/* several lines of code */
}
```
Restructured:
```golang
if ! condition {
continue
}
/* several lines of code */
```
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -166,21 +181,39 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
if !strings.HasPrefix(*cache.ServerType, string(tc.CacheTypeEdge)) {
continue
}
- if len(stats.KBPS) < 1 || len(stats.MaxKBPS) < 1 {
- continue
+ var kbps, maxKbps float64
+ for intName, intStats := range stats {
+ if len(intStats.KBPS) < 1 || len(intStats.MaxKBPS) < 1 {
+ continue
+ }
+ if checkServiceInterface(tx, intName, cacheName) {
Review comment:
Can `checkServiceInterface()` be rewritten so it only needs to be called a single time instead of once per cache?
##########
File path: lib/go-tc/enum.go
##########
@@ -55,6 +55,9 @@ type DeliveryServiceName string
// CacheType is the type (or tier) of a CDN cache.
type CacheType string
+// InterfaceName is the name of the interface
Review comment:
This doesn't help a user who doesn't already know what interfaces are and how they are used. Maybe *server interface*?
----------------------------------------------------------------
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:
users@infra.apache.org