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 2022/06/22 15:00:00 UTC
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6859: T3c add parent retry params for first./inner./last.
rob05c commented on code in PR #6859:
URL: https://github.com/apache/trafficcontrol/pull/6859#discussion_r903097327
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
Review Comment:
What is s?
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -386,214 +403,231 @@ func makeParentDotConfigData(
continue
}
- if txt != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
- parentAbstraction.Services = append(parentAbstraction.Services, txt)
+ if pasvc != nil { // will be nil with no error if this server isn't in the Topology, or if it doesn't have the Required Capabilities
+ parentAbstraction.Services = append(parentAbstraction.Services, pasvc)
}
- } else if cacheIsTopLevel {
- parentQStr := false
- if dsParams.QueryStringHandling == "" && dsParams.Algorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore != nil && tc.QStringIgnore(*ds.QStringIgnore) == tc.QStringIgnoreUseInCacheKeyAndPassUp {
- parentQStr = true
- }
-
- orgFQDNStr := *ds.OrgServerFQDN
- // if this cache isn't the last tier, i.e. we're not going to the origin, use http not https
- if isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds); !isLastCacheTier {
- orgFQDNStr = strings.Replace(orgFQDNStr, `https://`, `http://`, -1)
- }
- orgURI, orgWarns, err := getOriginURI(orgFQDNStr)
- warnings = append(warnings, orgWarns...)
- if err != nil {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' has malformed origin URI: '"+orgFQDNStr+"': skipping!"+err.Error())
- continue
+ } else {
+ isLastCacheTier := noTopologyServerIsLastCacheForDS(server, &ds)
+ serverPlacement := TopologyPlacement{
+ IsLastCacheTier: isLastCacheTier,
+ IsFirstCacheTier: !isLastCacheTier,
Review Comment:
This isn't correct for go-direct DSes. I'm not seeing a "IsFirst" equivalent, we probably need to add one, which is just whether it's an EDGE-type for non-topology:
```
func noTopologyServerIsFirstCacheForDS(server *Server, ds *DeliveryService) bool {
return strings.HasPrefix(server.Type, tc.EdgeTypePrefix)
}
```
Alternatively, at this point it might be worth adding a func to return a `TopologyPlacement` for non-Topology DSes.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
+ var warnings []string
+ hasValues := false
+
+ if v, ok := dsParams[keys.Algorithm]; ok && strings.TrimSpace(v) != "" {
+ policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
+ if policy != ParentAbstractionServiceRetryPolicyInvalid {
+ dsp.Algorithm = policy
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.Algorithm+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.ParentRetry]; ok {
+ dsp.ParentRetry = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.MaxSimpleRetries]; ok {
+ dsp.MaxSimpleRetries = v
+ hasValues = true
+ }
+ if v, ok := dsParams[keys.MaxUnavailableRetries]; ok {
+ dsp.MaxUnavailableServerRetries = v
+ hasValues = true
+ }
+
+ if v, ok := dsParams[keys.SimpleRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.SimpleServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.SimpleRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ if v, ok := dsParams[keys.UnavailableRetryResponses]; ok {
+ if unavailableServerRetryResponsesValid(v) {
+ dsp.UnavailableServerRetryResponses = v
+ hasValues = true
+ } else {
+ warnings = append(warnings, "DS '"+dsid+"' had malformed "+keys.UnavailableRetryResponses+" parameter '"+v+"', not using!")
+ }
+ }
+
+ // Unfortunately this may be s
+ if v, ok := dsParams[keys.SecondaryMode]; ok {
+ if v == "false" {
+ dsp.TryAllPrimariesBeforeSecondary = false
+ } else {
+ dsp.TryAllPrimariesBeforeSecondary = true
+ if v != "" {
+ warnings = append(warnings, "DS '"+dsid+"' had Parameter "+keys.SecondaryMode+" which is used if it exists, the value is ignored (unless false) ! Non-empty value '"+v+"' will be ignored!")
+ }
+ }
+ }
+
+ return hasValues, warnings
}
// getDSParams returns the Delivery Service Profile Parameters used in parent.config, and any warnings.
// If Parameters don't exist, defaults are returned. Non-MSO Delivery Services default to no custom retry logic (we should reevaluate that).
// Note these Parameters are only used for MSO for legacy DeliveryServiceServers DeliveryServices.
// Topology DSes use them for all DSes, MSO and non-MSO.
-func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string) (parentDSParams, []string) {
+func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]map[string]string, serverPlacement TopologyPlacement, isMSO bool) (parentDSParams, []string) {
warnings := []string{}
params := parentDSParams{}
- isMSO := ds.MultiSiteOrigin != nil && *ds.MultiSiteOrigin
- if isMSO {
+
+ // Default values
+ if serverPlacement.IsLastCacheTier && isMSO {
params.Algorithm = ParentConfigDSParamDefaultMSOAlgorithm
params.ParentRetry = ParentConfigDSParamDefaultMSOParentRetry
params.UnavailableServerRetryResponses = ParentConfigDSParamDefaultMSOUnavailableServerRetryResponses
params.MaxSimpleRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxSimpleRetries)
params.MaxUnavailableServerRetries = strconv.Itoa(ParentConfigDSParamDefaultMaxUnavailableServerRetries)
+ params.HasRetryParams = true
Review Comment:
I'm guessing this is true because we're "faking" the params? If so, would you mind adding a comment explaining that? It's not clear to me how and why it can have params by default, when no parameters exist.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -802,32 +837,100 @@ type originURI struct {
const deliveryServicesAllParentsKey = "all_parents"
type parentDSParams struct {
- Algorithm ParentAbstractionServiceRetryPolicy
+ Algorithm ParentAbstractionServiceRetryPolicy
+ QueryStringHandling string
+
+ HasRetryParams bool
ParentRetry string
- UnavailableServerRetryResponses string
MaxSimpleRetries string
MaxUnavailableServerRetries string
- QueryStringHandling string
+ SimpleServerRetryResponses string
+ UnavailableServerRetryResponses string
TryAllPrimariesBeforeSecondary bool
- UsePeering bool
- MergeGroups []string
+
+ UsePeering bool
+ MergeGroups []string
+}
+
+func (dsp *parentDSParams) FillParentRetries(keys ParentConfigRetryKeys, dsParams map[string]string, dsid string) (bool, []string) {
Review Comment:
Mind adding a function-level GoDoc comment for this? Particularly documenting its return values?
It's possible to figure out from reading the code, but ideally someone shouldn't have to.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -840,68 +943,50 @@ func getParentDSParams(ds DeliveryService, profileParentConfigParams map[string]
params.UsePeering = true
}
}
+
// the following may be blank, no default
params.QueryStringHandling = dsParams[ParentConfigParamQStringHandling]
params.MergeGroups = strings.Split(dsParams[ParentConfigParamMergeGroups], " ")
- // TODO deprecate & remove "mso." Parameters - there was never a reason to restrict these settings to MSO.
- if isMSO {
- if v, ok := dsParams[ParentConfigParamMSOAlgorithm]; ok && strings.TrimSpace(v) != "" {
- policy := ParentSelectAlgorithmToParentAbstractionServiceRetryPolicy(v)
- if policy != ParentAbstractionServiceRetryPolicyInvalid {
- params.Algorithm = policy
- } else {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOAlgorithm+" parameter '"+v+"', not using!")
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOParentRetry]; ok {
- params.ParentRetry = v
- }
- if v, ok := dsParams[ParentConfigParamMSOUnavailableServerRetryResponses]; ok {
- if v != "" && !unavailableServerRetryResponsesValid(v) {
- warnings = append(warnings, "DS '"+*ds.XMLID+"' had malformed "+ParentConfigParamMSOUnavailableServerRetryResponses+" parameter '"+v+"', not using!")
- } else if v != "" {
- params.UnavailableServerRetryResponses = v
- }
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxSimpleRetries]; ok {
- params.MaxSimpleRetries = v
- }
- if v, ok := dsParams[ParentConfigParamMSOMaxUnavailableServerRetries]; ok {
- params.MaxUnavailableServerRetries = v
+ // Secondary Mode should really be done per tier
Review Comment:
I'm not clear on what this comment is saying. Is it saying it's about to be? Or that it isn't, but it should be?
Would you mind clarifying to e.g. `// TODO set Secondary Mode per tier` or `// secondary mode is set per-tier`?
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org