[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379176893
##
File path: traffic_ops/traffic_ops_golang/crconfig/servers.go
##
@@ -111,7 +111,7 @@ func getAllServers(cdn string, tx *sql.Tx)
(map[string]ServerUnion, error) {
// TODO select deliveryservices as array?
q := `
-select s.host_name, cg.name as cachegroup, concat(s.host_name, '.',
s.domain_name) as fqdn, s.xmpp_id as hashid, s.https_port, s.interface_name,
s.ip_address, s.ip6_address, s.tcp_port, p.name as profile_name,
cast(p.routing_disabled as int), st.name as status, t.name as type
+select s.host_name, cg.name as cachegroup, concat(s.host_name, '.',
s.domain_name) as fqdn, s.xmpp_id as hashid, s.https_port, s.interface_name,
case when s.ip_address_is_service = true then s.ip_address else '' end, case
when s.ip6_address_is_service = true then s.ip6_address else '' end,
s.tcp_port, p.name as profile_name, cast(p.routing_disabled as int), st.name as
status, t.name as type
Review comment:
How strongly do you feel about this, vs an `if` statement in Go?
The `case` here is a conditional. I know this feels like a nitpick, but in
my experience, embedding code in the database or queries leads to pain. I think
if we keep code as code, and data as data, and SQL as a declarative query, it
just makes everything easier to understand and work with, and less to
misunderstand or go wrong.
Here, the `case` is technically SQL, but it's an imperative `if` statement,
not declarative like SQL is intended to be. It's not a query, it's logic.
Would you object to changing this to something like:
```
qry := `
SELECT
s.host_name,
cg.name AS cachegroup,
CONCAT(s.host_name, '.', s.domain_name) AS fqdn,
s.xmpp_id AS hashid,
s.https_port,
s.interface_name,
s.ip_address_is_service,
s.ip6_address_is_service,
s.ip_address,
s.ip6_address,
s.tcp_port,
p.name AS profile_name,
CAST(p.routing_disabled AS int),
st.name AS status,
t.name AS type
FROM
server s
JOIN cachegroup cg ON cg.id = s.cachegroup
JOIN type t on t.id = s.type
JOIN profile p ON p.id = s.profile
JOIN status st ON st.id = s.status
WHERE
cdn_id = (SELECT id FROM cdn WHERE name = $1)
AND (st.name = 'REPORTED' or st.name = 'ONLINE' or st.name = 'ADMIN_DOWN')
`
rows, err := tx.Query(qry, cdn)
if err != nil {
return nil, errors.New("Error querying servers: " + err.Error())
}
defer rows.Close()
for rows.Next() {
port := sql.NullInt64{}
ip6 := sql.NullString{}
hashId := sql.NullString{}
httpsPort := sql.NullInt64{}
ipIsService := false
ipIsService := false
host := ""
status := ""
s := ServerUnion{}
if err := rows.Scan(, , , ,
, , , , , , ,
, , , ); err != nil {
return nil, errors.New("Error scanning server: " +
err.Error())
}
if !ipIsService {
s.Ip = util.StrPtr("")
}
if !ip6IsService {
s.Ip6 = util.StrPtr("")
}
```
Really, the very need for imperative logic here is a result of
denormalization. But we certainly don't have time to properly normalize our
schema right now.
I know my position is a little bit nuanced. If you feel strongly in the
other direction, I won't push for it. But if it's all the same to you, IMO the
separation of data vs code makes things easier and safer to work with.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379169003
##
File path: traffic_monitor/poller/cache.go
##
@@ -162,14 +181,30 @@ func poller(
pollID := atomic.AddUint64(, 1)
pollFinishedChan := make(chan uint64)
log.Debugf("poll %v %v start\n", pollID, time.Now())
- bts, reqEnd, reqTime, err := pollFunc(pollCtx, url,
host, pollID)
- rdr := io.Reader(nil)
- if bts != nil {
- rdr = bytes.NewReader(bts) // TODO change
handler to take bytes? Benchmark?
+ if usingIPv4 {
+ bts, reqEnd, reqTime, err := pollFunc(pollCtx,
url, host, pollID)
Review comment:
It looks like this and the next block are identical, except for `url`. Could
the duplication be reduced by changing this to
```
pollURL := url
if !usingIPv4 {
pollURL = url6
}
bts, reqEnd, reqTime, err := pollFunc(pollCtx, pollURL, host, pollID)
```
?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379160057
##
File path: traffic_monitor/health/cache.go
##
@@ -171,45 +172,100 @@ func EvalCache(result cache.ResultInfo, resultStats
*threadsafe.ResultStatValHis
}
if !inThreshold(threshold, resultStatNum) {
- return false, eventDesc(status,
exceedsThresholdMsg(stat, threshold, resultStatNum)), stat
+ return false, result.UsingIPv4, eventDesc(status,
exceedsThresholdMsg(stat, threshold, resultStatNum)), stat
}
}
- return avail, eventDescVal, eventMsg
+ return avail, result.UsingIPv4, eventDescVal, eventMsg
}
// CalcAvailabilityWithStats calculates the availability of each cache in
results.
// statResultHistory may be nil, in which case stats won't be used to
calculate availability.
-func CalcAvailability(results []cache.Result, pollerName string,
statResultHistory *threadsafe.ResultStatHistory, mc tc.TrafficMonitorConfigMap,
toData todata.TOData, localCacheStatusThreadsafe
threadsafe.CacheAvailableStatus, localStates peer.CRStatesThreadsafe, events
ThreadsafeEvents) {
+func CalcAvailability(results []cache.Result, pollerName string,
statResultHistory *threadsafe.ResultStatHistory, mc tc.TrafficMonitorConfigMap,
toData todata.TOData, localCacheStatusThreadsafe
threadsafe.CacheAvailableStatus, localStates peer.CRStatesThreadsafe, events
ThreadsafeEvents, protocol config.PollingProtocol) {
localCacheStatuses := localCacheStatusThreadsafe.Get().Copy()
statResults := (*threadsafe.ResultStatValHistory)(nil)
+ processAvailableTuple := func(tuple cache.AvailableTuple, serverInfo
tc.TrafficServer) bool {
+ switch protocol {
+ case config.IPv4Only:
+ return tuple.IPv4
+ case config.IPv6Only:
+ return tuple.IPv6
+ case config.Both:
+ // only report availability based on defined IP
addresses
+ if serverInfo.IP == "" {
+ return tuple.IPv6
+ } else if serverInfo.IP6 == "" {
+ return tuple.IPv4
+ }
+ // if both IP addresses are defined then report
availability based on both
+ return tuple.IPv4 || tuple.IPv6
Review comment:
This returns that the cache is healthy if _either_ IPv4 _or_ IPv6 polling
reported available? Is that intentional?
If so, I think we should document that. I didn't see anything in the docs
indicating that "both" is really "either."
It'd be nice if it were a config option, for "both" and "either." But I
don't want to ask you to do more work, this works as-is, as long as we document
the behavior. We can always add "either" later.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379043530
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
It isn't. But that's not a bad idea, might be worth looking into. I don't
know how the cost of parsing the IP compares to the request cost; but we do
enough requests, and use enough CPU, it might be significant.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379024121
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
I wanted to be sure, so I wrote a benchmark:
```
package main
import (
"math/rand"
"net"
"regexp"
"strconv"
"strings"
"testing"
)
func BenchmarkRemoveCIDRFunc(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = ipv6CIDRStrToAddr(cidrStrs[i])
}
}
func BenchmarkRemoveCIDRRegex(b *testing.B) {
re := regexp.MustCompile(`/\d+`)
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = re.ReplaceAllString(cidrStrs[i], "")
}
}
func BenchmarkRemoveCIDRIP(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, err := net.ParseCIDR(cidrStrs[i])
if err != nil {
b.Fatal("not an IP: '" + cidrStrs[i] + "'")
}
ipStrs[i] = ip.String()
}
}
func BenchmarkRemoveCIDRIPNoErrCheck(b *testing.B) {
// because the others don't check for malformed input, it wasn't really
fair for the IP bench to do so if it's slower.
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, _ := net.ParseCIDR(cidrStrs[i])
ipStrs[i] = ip.String()
}
}
func randCIDRStrs(n int) []string {
ips := []net.IP{}
for i := 0; i < n; i++ {
ip := make([]byte, 16, 16)
rand.Read(ip) // math/rand.Read is documented to always return
a nil error.
ips = append(ips, net.IP(ip))
}
ipStrs := []string{}
for _, ip := range ips {
ipStrs = append(ipStrs,
ip.String()+`/`+strconv.Itoa(rand.Intn(129)))
}
return ipStrs
}
// ipv6CIDRStrToAddr takes an IPv6 CIDR string, e.g. `2001:DB8::1/32`
returns `2001:DB8::1`.
// It does not verify cidr is a valid CIDR or IPv6. It only removes the
first slash and everything after it, for performance.
func ipv6CIDRStrToAddr(cidr string) string {
i := strings.Index(cidr, `/`)
if i == -1 {
return cidr
}
return cidr[:i]
}
```
```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/apache/trafficcontrol/traffic_monitor
BenchmarkRemoveCIDRFunc-16 63100605 23.7 ns/op
BenchmarkRemoveCIDRRegex-16 4373604 290 ns/op
BenchmarkRemoveCIDRIP-16 2342938 530 ns/op
BenchmarkRemoveCIDRIPNoErrCheck-16 2322907 513 ns/op
PASS
ok github.com/apache/trafficcontrol/traffic_monitor37.686s
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379024121
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
I wanted to be sure, so I wrote a benchmark:
```
package main
import (
"math/rand"
"net"
"regexp"
"strconv"
"strings"
"testing"
)
func BenchmarkRemoveCIDRFunc(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = ipv6CIDRStrToAddr(cidrStrs[i])
}
}
func BenchmarkRemoveCIDRRegex(b *testing.B) {
re := regexp.MustCompile(`/\d+`)
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = re.ReplaceAllString(cidrStrs[i], "")
}
}
func BenchmarkRemoveCIDRIP(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, err := net.ParseCIDR(cidrStrs[i])
if err != nil {
b.Fatal("not an IP: '" + cidrStrs[i] + "'")
}
ipStrs[i] = ip.String()
}
}
func BenchmarkRemoveCIDRIPNoErrCheck(b *testing.B) {
// because the others don't check for malformed input, it wasn't really
fair for the IP bench to do so if it's slower.
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, _ := net.ParseCIDR(cidrStrs[i])
ipStrs[i] = ip.String()
}
}
func randCIDRStrs(n int) []string {
ips := []net.IP{}
for i := 0; i < n; i++ {
ip := make([]byte, 16, 16)
rand.Read(ip) // math/rand.Read is documented to always return
a nil error.
ips = append(ips, net.IP(ip))
}
ipStrs := []string{}
for _, ip := range ips {
ipStrs = append(ipStrs,
ip.String()+`/`+strconv.Itoa(rand.Intn(129)))
}
return ipStrs
}
// ipv6CIDRStrToAddr takes an IPv6 CIDR string, e.g. `2001:DB8::1/32`
returns `2001:DB8::1`.
// It does not verify cidr is a valid CIDR or IPv6. It only removes the
first slash and everything after it, for performance.
func ipv6CIDRStrToAddr(cidr string) string {
i := strings.Index(cidr, `/`)
if i == -1 {
return cidr
}
return cidr[:i]
}
```
```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/apache/trafficcontrol/traffic_monitor
BenchmarkRemoveCIDRFunc-16 6310060523.7 ns/op
BenchmarkRemoveCIDRRegex-16 4373604 290 ns/op
BenchmarkRemoveCIDRIP-16 2342938 530 ns/op
BenchmarkRemoveCIDRIPNoErrCheck-16 2322907 513 ns/op
PASS
ok github.com/apache/trafficcontrol/traffic_monitor37.686s
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379024121
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
I wanted to be sure, so I wrote a benchmark:
```
package main
import (
"math/rand"
"net"
"regexp"
"strconv"
"strings"
"testing"
)
func BenchmarkRemoveCIDRFunc(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = ipv6CIDRStrToAddr(cidrStrs[i])
}
}
func BenchmarkRemoveCIDRRegex(b *testing.B) {
re := regexp.MustCompile(`/\d+`)
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = re.ReplaceAllString(cidrStrs[i], "")
}
}
func BenchmarkRemoveCIDRIP(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, err := net.ParseCIDR(cidrStrs[i])
if err != nil {
b.Fatal("not an IP: '" + cidrStrs[i] + "'")
}
ipStrs[i] = ip.String()
}
}
func BenchmarkRemoveCIDRIPNoErrCheck(b *testing.B) {
// because the others don't check for malformed input, it wasn't really
fair for the IP bench to do so if it's slower.
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, _ := net.ParseCIDR(cidrStrs[i])
ipStrs[i] = ip.String()
}
}
func randCIDRStrs(n int) []string {
ips := []net.IP{}
for i := 0; i < n; i++ {
ip := make([]byte, 16, 16)
rand.Read(ip) // math/rand.Read is documented to always return
a nil error.
ips = append(ips, net.IP(ip))
}
ipStrs := []string{}
for _, ip := range ips {
ipStrs = append(ipStrs,
ip.String()+`/`+strconv.Itoa(rand.Intn(129)))
}
return ipStrs
}
// ipv6CIDRStrToAddr takes an IPv6 CIDR string, e.g. `2001:DB8::1/32`
returns `2001:DB8::1`.
// It does not verify cidr is a valid CIDR or IPv6. It only removes the
first slash and everything after it, for performance.
func ipv6CIDRStrToAddr(cidr string) string {
i := strings.Index(cidr, `/`)
if i == -1 {
return cidr
}
return cidr[:i]
}
```
```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/apache/trafficcontrol/traffic_monitor
BenchmarkRemoveCIDRFunc-16 6310060523.7 ns/op
BenchmarkRemoveCIDRRegex-16 4373604 290 ns/op
BenchmarkRemoveCIDRIP-16 2342938 530 ns/op
BenchmarkRemoveCIDRIPNoErrCheck-16 2322907 513 ns/op
PASS
ok github.com/apache/trafficcontrol/traffic_monitor37.686s
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r379024121
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
I wanted to be sure, so I wrote a benchmark:
```
package main
import (
"math/rand"
"net"
"regexp"
"strconv"
"strings"
"testing"
)
func BenchmarkRemoveCIDRFunc(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = ipv6CIDRStrToAddr(cidrStrs[i])
}
}
func BenchmarkRemoveCIDRRegex(b *testing.B) {
re := regexp.MustCompile(`/\d+`)
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ipStrs[i] = re.ReplaceAllString(cidrStrs[i], "")
}
}
func BenchmarkRemoveCIDRIP(b *testing.B) {
cidrStrs := randCIDRStrs(b.N)
ipStrs := make([]string, len(cidrStrs), len(cidrStrs))
b.ResetTimer()
for i := 0; i < b.N; i++ {
ip, _, err := net.ParseCIDR(cidrStrs[i])
if err != nil {
b.Fatal("not an IP: '" + cidrStrs[i] + "'")
}
ipStrs[i] = ip.String()
}
}
func randCIDRStrs(n int) []string {
ips := []net.IP{}
for i := 0; i < n; i++ {
ip := make([]byte, 16, 16)
rand.Read(ip) // math/rand.Read is documented to always return
a nil error.
ips = append(ips, net.IP(ip))
}
ipStrs := []string{}
for _, ip := range ips {
ipStrs = append(ipStrs,
ip.String()+`/`+strconv.Itoa(rand.Intn(129)))
}
return ipStrs
}
// ipv6CIDRStrToAddr takes an IPv6 CIDR string, e.g. `2001:DB8::1/32`
returns `2001:DB8::1`.
// It does not verify cidr is a valid CIDR or IPv6. It only removes the
first slash and everything after it, for performance.
func ipv6CIDRStrToAddr(cidr string) string {
i := strings.Index(cidr, `/`)
if i == -1 {
return cidr
}
return cidr[:i]
}
```
```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/apache/trafficcontrol/traffic_monitor
BenchmarkRemoveCIDRFunc-16 6310060523.7 ns/op
BenchmarkRemoveCIDRRegex-16 4373604 290 ns/op
BenchmarkRemoveCIDRIP-16 2069672 528 ns/op
PASS
ok github.com/apache/trafficcontrol/traffic_monitor37.686s
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r377383074
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
`net.ParseCIDR` is expensive too, though, and doing a lot of work that we
don't need 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches,
including monitoring and routing
URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r377374759
##
File path: traffic_monitor/manager/manager.go
##
@@ -45,6 +46,8 @@ import (
func Start(opsConfigFile string, cfg config.Config, appData
config.StaticAppData, trafficMonitorConfigFileName string) error {
toSession :=
towrap.ITrafficOpsSession(towrap.NewTrafficOpsSessionThreadsafe(nil,
cfg.CRConfigHistoryCount, cfg))
+ url6Regex := regexp.MustCompile(`/\d+`)
Review comment:
Regexes are typically much slower than simple string manipulation, and TM is
highly performance-sensitive. This isn't actually in the request path; but it
does get called for every server, every time a new config is received.
Would you object to changing the usage to a string function? E.g.
```
// ipv6CIDRStrToAddr takes an IPv6 CIDR string, e.g. `2001:DB8::1/32`
returns `2001:DB8::1`.
// It does not verify cidr is a valid CIDR or IPv6. It only removes the
first slash and everything after it, for performance.
func ipv6CIDRStrToAddr(cidr string) string {
i := strings.Index(cidr, `/`)
if i == -1 {
return cidr
}
return cidr[:i]
}
```
https://play.golang.org/p/1tLFJ7P1C91
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing
rob05c commented on a change in pull request #4142: Allow IPv6 Only Caches, including monitoring and routing URL: https://github.com/apache/trafficcontrol/pull/4142#discussion_r377365058 ## File path: traffic_monitor/manager/monitorconfig.go ## @@ -146,11 +148,12 @@ func StartMonitorConfigManager( staticAppData, toSession, toData, + url6Regex, ) return monitorConfig } -const DefaultHealthConnectionTimeout = time.Second * 2 +const DefaultHealthConnectionTimeout = time.Second * 20 Review comment: What's the thinking behind this? That's a pretty big jump 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: us...@infra.apache.org With regards, Apache Git Services
