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/02/20 20:21:42 UTC
[GitHub] [trafficcontrol] rob05c opened a new pull request #4419: ORT Speed
Improvements
rob05c opened a new pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419
## What does this PR (Pull Request) do?
This makes a number of changes to speed up ORT runs.
- changes `atstccfg` to not retry 404's from Traffic Ops. If TO returns a 404, it's not going to succeed on retries. With the exponential backoff retry, this saves about a minute on a TO missing Server Capabilities
- changes the default `atstccfg` cache format to Gob instead of JSON. This cuts the time of a total ORT run in about half, and for an individual cache file, reduces the parse time to about 1/5 the JSON time.
- changes Delivery Service Servers cache format to CSV, which takes about half the time of Gob, and is by far the largest file to load and parse.
- Changes the default cache time to 15m instead of 1m. The time was always intended to be a full ORT run, and 1m is far too short. This also exposes the flag in ORT itself, so users can change it if they want. It also changes ORT to delete the cache when it starts, to ensure the `atstccfg` cache is never reused by different ORT runs.
- Incidentally, this also abstracts the cache encoding, making it very easy for anyone to change or add new cache formats, if they find something faster, or prefer the easier-to-debug JSON. I left the JSON encoder in, and it's a 1-line compile-time change. I also tested CBOR, which is preferable to Gob as a standard, but it was over twice as slow as Gob, and `atstccfg` currently has 0 external dependencies, so I decided not to include it; but it's trivial to add.
Altogether, this cuts the new ORT total run time to about 1/3 what it was, and puts it close to the time it took before the `atstccfg` client-side config generation was added.
Includes tests. No docs, no changelog, no interface change.
- [x] This PR is not related to any other Issue
## Which Traffic Control components are affected by this PR?
- Traffic Ops ORT
## What is the best way to verify this PR?
Run unit tests. Run ORT, verify it still creates correct config files.
## If this is a bug fix, what versions of Traffic Control are affected?
Not a bug fix.
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382287471
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
Review comment:
Not entirely sure I agree adding a global variable increases the overall robustness. But, I changed it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382240955
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -84,17 +105,16 @@ func WriteCacheJSON(tempDir string, cacheFileName string, obj interface{}) {
}
defer objFile.Close()
- if _, err := objFile.Write(objBts); err != nil {
- log.Errorln("writing object cache file '" + objPath + "': " + err.Error())
+ if err := encode(objFile, obj); err != nil {
Review comment:
This does not check encode for nullity. Should a null encoder use JSON?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382259237
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
Review comment:
There's a lot of string allocation that is totally unnecessary here.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] dneuman64 merged pull request #4419: ORT Speed
Improvements
Posted by GitBox <gi...@apache.org>.
dneuman64 merged pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382271542
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -84,17 +105,16 @@ func WriteCacheJSON(tempDir string, cacheFileName string, obj interface{}) {
}
defer objFile.Close()
- if _, err := objFile.Write(objBts); err != nil {
- log.Errorln("writing object cache file '" + objPath + "': " + err.Error())
+ if err := encode(objFile, obj); err != nil {
Review comment:
If it's nil it should probably be the default, which is currently Gob.
But, I'd rather it logged/returned an error. I think requiring a valid value is safer and much easier to debug, than defaulting when the nil pointer could be an accident.
But yes, the panic is bad.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382274716
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
+ if err != nil {
+ if err == io.EOF {
+ return nil
+ }
+ return errors.New("malformed object file:" + err.Error())
+ }
+ dsStr = dsStr[:len(dsStr)-1] // ReadString(',') includes the comma; remove it
+
+ ds, err := strconv.Atoi(dsStr)
+ if err != nil {
+ return errors.New("malformed object file: first field should be ds id, but is not an integer: '" + dsStr + "'")
+ }
+ svStr, err := objFileReader.ReadString('\n')
+ if err != nil {
+ if err == io.EOF {
+ return errors.New("malformed object file: final line had ds but not server")
Review comment:
Which will never happen unless someone manually modifies the cache file. But, I changed the message to be more informative.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382259939
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
+ if err != nil {
+ if err == io.EOF {
+ return nil
+ }
+ return errors.New("malformed object file:" + err.Error())
+ }
+ dsStr = dsStr[:len(dsStr)-1] // ReadString(',') includes the comma; remove it
+
+ ds, err := strconv.Atoi(dsStr)
+ if err != nil {
+ return errors.New("malformed object file: first field should be ds id, but is not an integer: '" + dsStr + "'")
+ }
+ svStr, err := objFileReader.ReadString('\n')
+ if err != nil {
+ if err == io.EOF {
+ return errors.New("malformed object file: final line had ds but not server")
Review comment:
This can also happen if the final line is missing the newline.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382273826
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -105,24 +125,42 @@ func GetJSONObjFromFile(tempDir string, cacheFileName string, cacheFileMaxAge ti
if err != nil {
return errors.New("getting object file info '" + objPath + "':" + err.Error())
}
-
if objFileAge := time.Now().Sub(objFileInfo.ModTime()); objFileAge > cacheFileMaxAge {
return fmt.Errorf("object file too old, max age %dms less than file age %dms", int(cacheFileMaxAge/time.Millisecond), int(objFileAge/time.Millisecond))
}
-
- bts, err := ioutil.ReadAll(objFile)
- if err != nil {
- return errors.New("reading object from file '" + objPath + "':" + err.Error())
- }
-
- if err := json.Unmarshal(bts, obj); err != nil {
+ if err := decode(objFile, obj); err != nil {
Review comment:
Changed to return an error, as above
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382262764
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+ my $real_use_cache = $use_cache;
+ $use_cache = 0;
Review comment:
This is a very sad way to pass an argument, even for perl. :(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382275040
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+ my $real_use_cache = $use_cache;
+ $use_cache = 0;
Review comment:
It's following the idiom of other arguments 🤷
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382265091
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
Review comment:
So, this only works because this is the first request of the run. If someone adds a call before this at some point, they could easily wind up introducing a very subtle race condition. It would be nice to see this a bit more robust? Maybe a global clear-cache flag that is checked and set by lwp_get itself that would run on first call regardless of what that was?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382256113
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -105,24 +125,42 @@ func GetJSONObjFromFile(tempDir string, cacheFileName string, cacheFileMaxAge ti
if err != nil {
return errors.New("getting object file info '" + objPath + "':" + err.Error())
}
-
if objFileAge := time.Now().Sub(objFileInfo.ModTime()); objFileAge > cacheFileMaxAge {
return fmt.Errorf("object file too old, max age %dms less than file age %dms", int(cacheFileMaxAge/time.Millisecond), int(objFileAge/time.Millisecond))
}
-
- bts, err := ioutil.ReadAll(objFile)
- if err != nil {
- return errors.New("reading object from file '" + objPath + "':" + err.Error())
- }
-
- if err := json.Unmarshal(bts, obj); err != nil {
+ if err := decode(objFile, obj); err != nil {
Review comment:
Likewise with the decoder. Should a nil encoder have a default meaning?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382268582
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
Review comment:
Nope, I tested, it's slower.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382267890
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
Review comment:
There's no `[]byte` to int conversion func in Go.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382303455
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1508,12 +1512,22 @@ sub lwp_get {
my ( $TO_USER, $TO_PASS ) = split( /:/, $TM_LOGIN );
+ # atstccfg_cache_cleared is a global variable we use to clear the atstccfg cache on the first atstccfg call.
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+
my $no_cache_arg = '';
- if ( $use_cache == 0 ) {
+ if ( $use_cache == 0 || $atstccfg_cache_cleared == 0 ) {
+ $atstccfg_cache_cleared = 1;
$no_cache_arg = '--no-cache';
}
- $response_content = `$atstccfg_cmd $no_cache_arg --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$request' --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
+ my $cache_age_arg='';
+ if (length $cache_max_age_seconds > 0) {
+ $cache_age_arg = "--cache-file-max-age-seconds=$cache_max_age_seconds";
+ }
+
+ $response_content = `$atstccfg_cmd $no_cache_arg $cache_age_arg --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$request' --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
Review comment:
This introduces an unnecessary space when you don't pass `--no-cache`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382267890
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
Review comment:
There's no `[]byte` to number conversion func in Go.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382257282
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
+ return fmt.Errorf("writing object cache file: " + err.Error())
+ }
+ }
+ return nil
+}
+
+// DSSDecode is a DecodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSDecode(r io.Reader, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ objFileReader := bufio.NewReader(r)
+ for {
+ dsStr, err := objFileReader.ReadString(',')
Review comment:
Why use strings here? Wouldn't []byte be faster?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382273711
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -84,17 +105,16 @@ func WriteCacheJSON(tempDir string, cacheFileName string, obj interface{}) {
}
defer objFile.Close()
- if _, err := objFile.Write(objBts); err != nil {
- log.Errorln("writing object cache file '" + objPath + "': " + err.Error())
+ if err := encode(objFile, obj); err != nil {
Review comment:
Ok, I changed it to log/return errors instead of panicing.
Heaven forbid we have the type safety of nonnullable functions.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382262764
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+ my $real_use_cache = $use_cache;
+ $use_cache = 0;
Review comment:
This is a very sad way to pass an argument, even for perl. :(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382262764
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1847,7 +1855,12 @@ sub get_cfg_file_list {
my $cdn_name;
my $uri = "/api/1.4/servers/$host_name/configfiles/ats";
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+ my $real_use_cache = $use_cache;
+ $use_cache = 0;
Review comment:
This is a very sad way to pass an argument, even for perl. :(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382305354
##########
File path: traffic_ops/ort/traffic_ops_ort.pl
##########
@@ -1508,12 +1512,22 @@ sub lwp_get {
my ( $TO_USER, $TO_PASS ) = split( /:/, $TM_LOGIN );
+ # atstccfg_cache_cleared is a global variable we use to clear the atstccfg cache on the first atstccfg call.
+ # Telling atstccfg to use-cache=false will cause it to delete the cache directory
+ # Which is what we want: when ORT starts to run, delete the cache. We only want to use the atstccfg cache within the same ORT run, not across different runs.
+
my $no_cache_arg = '';
- if ( $use_cache == 0 ) {
+ if ( $use_cache == 0 || $atstccfg_cache_cleared == 0 ) {
+ $atstccfg_cache_cleared = 1;
$no_cache_arg = '--no-cache';
}
- $response_content = `$atstccfg_cmd $no_cache_arg --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$request' --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
+ my $cache_age_arg='';
+ if (length $cache_max_age_seconds > 0) {
+ $cache_age_arg = "--cache-file-max-age-seconds=$cache_max_age_seconds";
+ }
+
+ $response_content = `$atstccfg_cmd $no_cache_arg $cache_age_arg --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$request' --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
Review comment:
It doesn't hurt anything though, the arg parser can handle it. IMO safer and easier to understand than the code to avoid it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [trafficcontrol] alficles commented on a change in pull request
#4419: ORT Speed Improvements
Posted by GitBox <gi...@apache.org>.
alficles commented on a change in pull request #4419: ORT Speed Improvements
URL: https://github.com/apache/trafficcontrol/pull/4419#discussion_r382256966
##########
File path: traffic_ops/ort/atstccfg/toreq/caching.go
##########
@@ -177,3 +215,65 @@ func GetCookiesFromFile(tempDir string, cacheFileMaxAge time.Duration) (string,
}
return string(bts), nil
}
+
+// DSSExtension is the file extension for the format read and written by DSSEncode and DSSDecode.
+const DSSExtension = ".csv"
+
+// DSSEncode is an EncodeFunc optimized for Delivery Service Servers.
+// If iObj is not a *[]tc.DeliveryServiceServer it immediately returns an error.
+func DSSEncode(w io.Writer, iObj interface{}) error {
+ // Please don't change this to use encoding/csv unless you benchmark and prove it's at least as fast. DSS is massive, and performance is important.
+ obj, ok := iObj.(*[]tc.DeliveryServiceServer)
+ if !ok {
+ return fmt.Errorf("object is '%T' must be a *[]tc.DeliveryServiceServer\n", iObj)
+ }
+ for _, dss := range *obj {
+ if dss.DeliveryService == nil || dss.Server == nil {
+ continue // TODO warn?
+ }
+ if _, err := io.WriteString(w, strconv.Itoa(*dss.DeliveryService)+`,`+strconv.Itoa(*dss.Server)+"\n"); err != nil {
Review comment:
Would this be faster with multiple writes instead of string concatenation?
----------------------------------------------------------------
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
With regards,
Apache Git Services