subject:"\[GitHub\] rawlinp commented on a change in pull request #1908\: Changes for Backup Edge Cache Group"

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r173260427
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -295,15 +296,15 @@ public Geolocation getLocation(final String clientIP, 
final DeliveryService deli
track.setResult(ResultType.MISS);

track.setResultDetails(ResultDetails.DS_CZ_ONLY);
}
-   } else {
+   } else if (track.isUseNextClosest()) {
+   //Even with Geo limit none, TR wont do geo look-up, if 
fallback is diabled via backupZones configuration
 
 Review comment:
   diabled -> disabled


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r173211541
 
 

 ##
 File path: CHANGELOG.md
 ##
 @@ -1,13 +1,18 @@
 # Changelog
 All notable changes to this project will be documented in this file.
+===
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ## [Unreleased]
 ### Added
 - Per-DeliveryService Routing Names: you can now choose a Delivery Service's 
Routing Name (rather than a hardcoded "tr" or "edge" name). This might require 
a few pre-upgrade steps detailed 
[here](http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/migration_from_20_to_22.html#per-deliveryservice-routing-names)
 
+- Backup Edge Cache group: Backup Edge group for a particular cache group can 
be configured using coverage zone files. With this, users will be able control 
traffic within portions of their network, there by avoiding choosing fall back 
cache groups using geo co-ordinates(getClosestAvailableCachegroup) This can be 
controlled using "backupZones" which contains configuration for backup cache 
groups parsed from Coverage zone file as explained [here] 
(http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/using.html#the-coverage-zone-file-and-asn-table)
+
+
 ### Changed
 - Reformatted this CHANGELOG file to the keep-a-changelog format
 
-[Unreleased]: 
https://github.com/apache/incubator-trafficcontrol/compare/RELEASE-2.1.0...HEAD
+-[Unreleased]: 
https://github.com/apache/incubator-trafficcontrol/compare/RELEASE-2.1.0...HEAD 
 
 Review comment:
   the change here breaks the `[Unreleased]` link above, this shouldn't be 
changed


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r173212082
 
 

 ##
 File path: CHANGELOG.md
 ##
 @@ -1,13 +1,18 @@
 # Changelog
 All notable changes to this project will be documented in this file.
+===
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ## [Unreleased]
 ### Added
 - Per-DeliveryService Routing Names: you can now choose a Delivery Service's 
Routing Name (rather than a hardcoded "tr" or "edge" name). This might require 
a few pre-upgrade steps detailed 
[here](http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/migration_from_20_to_22.html#per-deliveryservice-routing-names)
 
+- Backup Edge Cache group: Backup Edge group for a particular cache group can 
be configured using coverage zone files. With this, users will be able control 
traffic within portions of their network, there by avoiding choosing fall back 
cache groups using geo co-ordinates(getClosestAvailableCachegroup) This can be 
controlled using "backupZones" which contains configuration for backup cache 
groups parsed from Coverage zone file as explained [here] 
(http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/using.html#the-coverage-zone-file-and-asn-table)
+
+
 
 Review comment:
   based on the keepachangelog format there should only be one blank line 
between sections


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r173208586
 
 

 ##
 File path: CHANGELOG.md
 ##
 @@ -1,13 +1,18 @@
 # Changelog
 All notable changes to this project will be documented in this file.
+===
 
 Review comment:
   this line is unnecessary, it turns the line above into a large header


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge
Cache Group
URL:
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r173208939

##
File path: CHANGELOG.md
##
@@ -1,13 +1,18 @@
# Changelog
All notable changes to this project will be documented in this file.
+===

The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

## [Unreleased]
### Added
- Per-DeliveryService Routing Names: you can now choose a Delivery Service's
Routing Name (rather than a hardcoded "tr" or "edge" name). This might require
a few pre-upgrade steps detailed
[here](http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/migration_from_20_to_22.html#per-deliveryservice-routing-names)

+- Backup Edge Cache group: Backup Edge group for a particular cache group can
be configured using coverage zone files. With this, users will be able control
traffic within portions of their network, there by avoiding choosing fall back
cache groups using geo co-ordinates(getClosestAvailableCachegroup) This can be
controlled using "backupZones" which contains configuration for backup cache
groups parsed from Coverage zone file as explained [here]
(http://traffic-control-cdn.readthedocs.io/en/latest/admin/traffic_ops/using.html#the-coverage-zone-file-and-asn-table)

Review comment:
A few things here:
1. Based on the keepachangelog format, there shouldn't be blank lines
between bullet points, so this line should be moved up.
2. "thereby" not "there by", "fallback" not "fall back", "coordinates" not
"co-ordinates"
3. A period is needed after "(getClosestAvailableCachegroup)"
4. "the Coverage Zone File" not "Coverage zone file"
5. Also, the `[here] (http://...)` link doesn't work if there's a space
between `[here]` and the link `(http://..)`

This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172944013
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -279,7 +282,11 @@ public Geolocation getLocation(final String clientIP, 
final DeliveryService deli
track.setResult(ResultType.MISS);

track.setResultDetails(ResultDetails.DS_CZ_ONLY);
}
-   } else {
+   } else if ((useDeep) || (networkNode != null && 
networkNode.isUseClosest()) || (networkNode == null)) {
 
 Review comment:
   I think this can remain a simple `else` if the logic in my comment above is 
implemented.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172958370
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +692,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupCacheGroups() != null) {
+   for (final String cacheGroup : 
networkNode.getBackupCacheGroups()) {
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(cacheGroup);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
+   }
+   }
+   return closestCacheLocation;
+   }
 
 Review comment:
   Following from my suggestions above, this line would probably be:
   ```
   } else {
   track.SetNoBackupLocationsAvailable(true);
   return null;
   }
   ```
   Then I'm not sure if we can also remove the `return null` statement below 
this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172938260
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -345,8 +352,9 @@ public DNSRouteResult route(final DNSRequest request, 
final Track track) throws
return result;
}
 
-   final CacheLocation cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds);
+   final CacheLocation cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds, track, false);
 
 Review comment:
   is it possible to change the method signature to have this be 
`getCoverageZoneCacheLocation(request.getClientIP(), ds, track);` so that we're 
not passing `useDeep` explicitly as false? `useDeep` should default to false.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172958519
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -262,7 +264,8 @@ public Geolocation getLocation(final String clientIP, 
final DeliveryService deli
}
} else {
// Deep caching not enabled for this Delivery Service; 
use the regular CZ
-   cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds);
+   cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds, track, useDeep);
+   networkNode = getNetworkNode(request.getClientIP());
 
 Review comment:
   Generally I'm not a big fan of polluting this method with extra NetworkNode 
stuff, especially since `getNetworkNode` is already called in 
`getCoverageZoneCacheLocation`. I think ideally `getCoverageZoneCacheLocation` 
should return us the information we need.
   
   Since we're passing in the `Track` object, we could probably add that 
information there, ie something like `track.noBackupLocationsAvailable()` which 
would get set within `getCoverageZoneCacheLocation`. Then we can update the 
logic starting on L277 to something like this:
   ```
   if (track.noBackupLocationsAvailable() or ds.isCoverageZoneOnly()) { 
   if (ds.geoRedirect) {...}
   else {
   resultDetails = DS_CZ_ONLY if ds.isCoverageZoneOnly else DS_CZ_NO_BKP
   ...
   }
   }
   ```
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172956208
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/StatTracker.java
 ##
 @@ -116,7 +116,7 @@ public void setRegionalAlternateCount(final int 
regionalAlternateCount) {
 
public enum ResultDetails {
NO_DETAILS, DS_NOT_FOUND, DS_TLS_MISMATCH, 
DS_NO_BYPASS, DS_BYPASS, DS_CZ_ONLY, DS_CLIENT_GEO_UNSUPPORTED, 
GEO_NO_CACHE_FOUND,
-   REGIONAL_GEO_NO_RULE, 
REGIONAL_GEO_ALTERNATE_WITHOUT_CACHE, REGIONAL_GEO_ALTERNATE_WITH_CACHE
+   REGIONAL_GEO_NO_RULE, 
REGIONAL_GEO_ALTERNATE_WITHOUT_CACHE, REGIONAL_GEO_ALTERNATE_WITH_CACHE, 
DS_CZ_BACKUP_CG
 
 Review comment:
   maybe add `DS_CZ_NO_BKP` to signify that the matched cachegroup and its 
configured backup cachegroups were unavailable.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172936794
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -262,7 +264,8 @@ public Geolocation getLocation(final String clientIP, 
final DeliveryService deli
}
} else {
// Deep caching not enabled for this Delivery Service; 
use the regular CZ
-   cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds);
+   cacheLocation = 
getCoverageZoneCacheLocation(request.getClientIP(), ds, track, useDeep);
 
 Review comment:
   Ideally this line should just pass `track` into this method and let the 
method default `useDeep` to false.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172912339
 
 

 ##
 File path: CHANGELOG.md
 ##
 @@ -1,3 +1,14 @@
+v2.3.0 [unreleased]
+---
+
+### Release Notes
+
+ Backup Edge Cache group
 
 Review comment:
   The changelog format has now changed to the keepachangelog format (there's 
now a link to that in the changelog), so you will need to rebase this branch 
onto the latest version of master and update these notes to use the new format.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

2018-03-06 Thread GitBox

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172619956
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupCacheGroups() != null) {
+   for (final String cacheGroup : 
networkNode.getBackupCacheGroups()) {
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(cacheGroup);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
+   }
+   }
+   return closestCacheLocation;
+   }
+   return null;
 
 Review comment:
   I agree with that, but the scenario I'm concerned about is when 
`fallbackToNextClosest == false`. In that case I don't think the request should 
fallback to Geo when the client was found in the CZF but the matched cachegroup 
has no caches available. In the best-case geo-lookup that client would still be 
traversing the same network path to the next available cachegroup (which is 
what the backup list is supposed to prevent).


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

2018-03-06 Thread GitBox

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172611695
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupCacheGroups() != null) {
+   for (final String cacheGroup : 
networkNode.getBackupCacheGroups()) {
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(cacheGroup);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
+   }
+   }
+   return closestCacheLocation;
+   }
+   return null;
 
 Review comment:
   Geo is generally the backup case when a client's IP cannot be found in the 
CZF. If the client is found in the CZF, we shouldn't be falling back to a 
Geo-lookup which subverts the CZF's `backupZones` config. We know the client is 
in our network, so we want it to follow the rules specified in the CZF.
   
   If the client was _not_ found in the CZF, then it should most likely _not_ 
be in our network. In that case, the congestion might not be a problem because 
the client should be entering our network from somewhere else.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

2018-03-06 Thread GitBox

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172585396
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupCacheGroups() != null) {
+   for (final String cacheGroup : 
networkNode.getBackupCacheGroups()) {
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(cacheGroup);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
+   }
+   }
+   return closestCacheLocation;
+   }
+   return null;
 
 Review comment:
   I disagree, one of the reasons for specifying backupZones is to confine 
traffic to certain cachegroups when the matched cachegroups are unavailable. In 
this case we're getting a hit in the CZ (so we know where the client is in the 
network), that cachegroup is unavailable, but the client will still fall back 
to a Geo-lookup (even though from the CZ-hit we already know where the client 
is), best-case scenario get matched to the same cachegroup, find the closest 
available cachegroup, and altogether disregard the "confine traffic to certain 
cachegroups" rule in the CZF.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

2018-03-05 Thread GitBox

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r172324766
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupCacheGroups() != null) {
+   for (final String cacheGroup : 
networkNode.getBackupCacheGroups()) {
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(cacheGroup);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
+   }
+   }
+   return closestCacheLocation;
+   }
+   return null;
 
 Review comment:
   Rather just returning `null` here, I think we have to return more 
information to the caller so that the caller knows the difference between "null 
because there wasn't a hit in the CZF" and "null because there _was_ a hit in 
the CZF but we're not returning a CacheLocation due to the `"backupZones"` 
config.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947866
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
 
 Review comment:
   `cachegroup` rather than `network`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171949234
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -198,12 +219,14 @@ public NetworkNode(final String str) throws 
NetworkNodeException {
 }
 
 public NetworkNode(final String str, final String loc) throws 
NetworkNodeException {
-this(str, loc, null);
+this(str, loc, null, null, false);
 }
 
-public NetworkNode(final String str, final String loc, final Geolocation 
geolocation) throws NetworkNodeException {
+public NetworkNode(final String str, final String loc, final Geolocation 
geolocation, final List backupCacheLoc, final boolean 
useClosestOnBackupFailure) throws NetworkNodeException {
 this.loc = loc;
 this.geolocation = geolocation;
+this.backupCacheLoc = backupCacheLoc;
 
 Review comment:
   `this.backupCacheGroups`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171956010
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
 
 Review comment:
   I think these two lines can be replaced with:
   ```
   useClosestOnBackupFailure = JsonUtils.optBoolean(backupConfigJson, 
"fallbackToClosestGroup", false);
   ```
   Then you also don't need the `fallbackNode` intermediate variable


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171951577
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupLoc() != null) {
+   for (final String element : networkNode.getBackupLoc()) 
{
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(element);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
 
 Review comment:
   in this case, it wasn't from backupCzGroup but the default behavior of 
nextClosest. Should we just omit this if-block?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171925592
 
 

 ##
 File path: docs/source/admin/traffic_ops/using.rst
 ##
 @@ -881,7 +899,7 @@ The CZF is an input to the Traffic Control CDN, and as 
such does not get generat
 
 The script that generates the CZF file is not part of Traffic Control, since 
it is different for each situation.
 
-.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically).
+.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically). The ``"backupZones"`` section is 
optional and is used by Traffic Router for localization in the case of a CZF 
"hit" when there are no caches available for that DS in the matched cache 
group.  This backup list is ordered and if there are no caches available in any 
of the configured backup cache groups, the request will be bypassed (if 
configured) or rejected or gets to the closest cache groups if configured 
(``"fallbackToClosestGroup"`` flag) for this backup configuration.
 
 Review comment:
   I would break this last sentence up to make it clearer (starting from "This 
backup list...":
   This backup ``"list"`` contains an ordered list of backup groups to choose 
from if the matched cache group has no caches available for a requested DS. If 
an available cache cannot be found in any of the backup groups either, the 
``"fallbackToClosestGroup"`` flag determines the Traffic Router's following 
behavior. If ``true``, Traffic Router will find the next closest cache group 
with available caches. If ``false`` (the default), Traffic Router will bypass 
the request (if configured) or reject it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171948607
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
+useClosestOnBackupFailure = (fallbackNode != null) ? 
fallbackNode.asBoolean(): false;
+} else {
+useClosestOnBackupFailure = true;
+}
+
+if (!addNetworkNodesToRoot(root, loc, locData, 
geolocation,bkCacheLoc, useDeep, useClosestOnBackupFailure)) {
 
 Review comment:
   nit: space after `geolocation,`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947182
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
 
 Review comment:
   kind of a nitpick but can we call this variable `backupConfigJson`? All the 
`Node` stuff is confusing with JsonNode vs NetworkNode.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950963
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupLoc() != null) {
+   for (final String element : networkNode.getBackupLoc()) 
{
 
 Review comment:
   `cachegroup` rather than `element`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950759
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -281,6 +304,14 @@ public Geolocation getGeolocation() {
 return geolocation;
 }
 
+public List getBackupLoc() {
 
 Review comment:
   `getBackupCacheGroups`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171948090
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 Review comment:
   `backupCacheGroups`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171920410
 
 

 ##
 File path: CHANGELOG.md
 ##
 @@ -1,3 +1,14 @@
+v2.3.0 [unreleased]
+---
+
+### Release Notes
+
+ Backup Edge Cache group
 
 Review comment:
   just as a heads up (you shouldn't have to take any action), I might be 
reformatting the changelog later per the mailing list discussion about using 
the keepachangelog.com format.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171952777
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
+useClosestOnBackupFailure = (fallbackNode != null) ? 
fallbackNode.asBoolean(): false;
+} else {
+useClosestOnBackupFailure = true;
 
 Review comment:
   I think we should just initialize `useClosestOnBackupFailure` to true on 
L113 and omit this if-else block. It doesn't change behavior; it would just be 
cleaner because it will be set to false in the previous block if configured 
anyway.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947198
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
 
 Review comment:
   I think this variable is unneeded; see comment on L131


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950588
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -137,18 +157,19 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 return null;
 }
 
+
+@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
 private static boolean addNetworkNodesToRoot(final SuperNode root, final 
String loc, final JsonNode locData,
- final Geolocation 
geolocation, final boolean useDeep) {
+ final Geolocation 
geolocation, final List bkCacheLoc, final boolean useDeep, final 
boolean useClosestOnBackupFailure) {
 
 Review comment:
   `backupCacheGroups` rather than `bkCacheLoc`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r170325325
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +107,24 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupList = locData.get("backupList");
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupList != null) {
+bkCacheLoc = new ArrayList<>();
 
 Review comment:
   too much indentation here


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r170325742
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -635,11 +638,11 @@ protected NetworkNode getNetworkNode(final String ip) {
}
 
public CacheLocation getCoverageZoneCacheLocation(final String ip, 
final String deliveryServiceId) {
-   return getCoverageZoneCacheLocation(ip, deliveryServiceId, 
false); // default is not deep
+   return getCoverageZoneCacheLocation(ip, deliveryServiceId, 
null, false); // default is not deep
}
 
@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
-   public CacheLocation getCoverageZoneCacheLocation(final String ip, 
final String deliveryServiceId, final boolean useDeep) {
+   public CacheLocation getCoverageZoneCacheLocation(final String ip, 
final String deliveryServiceId,final Track track, final boolean useDeep) {
 
 Review comment:
   missing space between parameters


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r170327632
 
 

 ##
 File path: docs/source/admin/traffic_ops/using.rst
 ##
 @@ -881,7 +892,9 @@ The CZF is an input to the Traffic Control CDN, and as 
such does not get generat
 
 The script that generates the CZF file is not part of Traffic Control, since 
it is different for each situation.
 
-.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically).
+.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically).  
+
+The ``"backupList"`` section is optional and is used by Traffic Router for 
localization in the case of a CZF "hit" when there are no caches available for 
that DS in the matched cache group.  This backup list is ordered and if there 
are no caches available in any of the configured backup cache groups, the 
request will be bypassed (if configured) or rejected
 
 Review comment:
   question: did you build the docs with this change to make sure the 
`backupList` note is contained within the same box as the `coordinates` note? 
The syntax highlighting here makes me think that the 2nd note might not be in 
the same box


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group