ocket8888 commented on code in PR #7660:
URL: https://github.com/apache/trafficcontrol/pull/7660#discussion_r1275370305
##########
lib/go-tc/federation_resolver.go:
##########
@@ -51,7 +51,7 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
-// FederationResolverV5 - is an alias for the Federal Resolver struct response
used for the latest minor version associated with APIv5.
+// FederationResolverV5 - is an alias for the Federal Resolver struct response
used for the latest major version associated with APIv5.
Review Comment:
This is not correct - `minor` is the right word here.
##########
lib/go-tc/deliveryservice_servers.go:
##########
@@ -290,3 +349,14 @@ func (baseV4 DSServerBaseV4)
ToDSServerBase(routerHostName, routerPort, pDesc *s
dsServerBase.RouterPortName = routerPort
return dsServerBase
}
+
+// ConvertV4LastupdateToV5 convert DSServerV4 lastUpdated time format to
RFC3339 for DSServerV5
+func ConvertV4LastupdateToV5(serverList []DSServerV4) []DSServerV5 {
+ updatedServerList := make([]DSServerV5, len(serverList))
+
+ for i, server := range serverList {
+ r := time.Unix(server.LastUpdated.Unix(), 0)
+ updatedServerList[i].LastUpdated = &r
+ }
+ return updatedServerList
+}
Review Comment:
So, what this function does, is it creates a new list of `DSServerV5`s where
each member of that array has the same `LastUpdated` time as the corresponding
member in the `DSServerV4` array, but just using `time.Time` (pointer) instead
of a `TimeNoMod` (pointer).
And that's it. It doesn't set any other field of the structs in the returned
array, so you get back no information at all _except_ the `LastUpdated` time. I
made a playground to illustrate the problem: https://go.dev/play/p/1X24ajmzLwE
##########
lib/go-tc/federation_resolver.go:
##########
@@ -72,7 +72,7 @@ type FederationResolversResponseV50 struct {
Response []FederationResolverV5 `json:"response"`
}
-// FederationResolverResponseV50 - represents struct response used for the
latest minor version associated with APIv5.
+// FederationResolverResponseV5 - represents struct response used for the
latest major version associated with APIv5.
Review Comment:
Same as above.
Correcting `FederationResolverResponseV50` to `FederationResolverV5` is
accurate, though.
##########
lib/go-tc/federation_resolver.go:
##########
@@ -63,7 +63,7 @@ type FederationResolverV50 struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
-// FederationResolversResponseV5 - an alias for the Federation Resolver's
struct response used for the latest minor version associated with APIv5.
+// FederationResolversResponseV5 - an alias for the Federation Resolver's
struct response used for the latest major version associated with APIv5.
Review Comment:
Same as above.
##########
traffic_ops/traffic_ops_golang/deliveryservice/eligible.go:
##########
@@ -42,6 +42,8 @@ func GetServersEligible(w http.ResponseWriter, r
*http.Request) {
}
defer inf.Close()
+ alerts := tc.Alerts{}
Review Comment:
This variable is declared 50 lines away from its only usage, which is a
read, so this variable doesn't need to exist at all. The value it is given
(which is never modified) could instead be used directly where that access is
being done currently.
##########
lib/go-tc/federation_resolver.go:
##########
@@ -104,7 +104,7 @@ func (fr *FederationResolver) Validate(tx *sql.Tx) error {
}
// UpgradeToFederationResolverV5 upgrades an APIv4 Federal Resolver into an
APIv5 Federal Resolver of
-// the latest minor version.
+// the latest major version APIv5.
Review Comment:
Same as above, also the <ins> APIv5</ins> you added should not have
been added.
##########
traffic_ops/traffic_ops_golang/deliveryservice/eligible.go:
##########
@@ -96,6 +98,17 @@ func GetServersEligible(w http.ResponseWriter, r
*http.Request) {
api.WriteResp(w, r, v3ServerList)
return
}
+
+ // Based on version we load Delivery Service Eligible Server - for
version 5 and above we use DSServerV5
+ if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0}) {
+
+ // Convert lastupdate time format to RFC3339 of DSServerV4 to
DSServerV5
+ newServerList := tc.ConvertV4LastupdateToV5(servers)
+
+ api.WriteAlertsObj(w, r, http.StatusOK, alerts, newServerList)
Review Comment:
`alerts` is always empty at this point, so why use `api.WriteAlertsObj`? You
don't have any alerts to write.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
