[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #3552: Draft: TO/TR Consistent Hash with Query Parameters

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #3552: Draft: TO/TR Consistent Hash with Query Parameters
URL: https://github.com/apache/trafficcontrol/pull/3552#discussion_r281742079
 
 

 ##########
 File path: traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##########
 @@ -559,9 +560,17 @@ public HTTPRouteResult multiRoute(final HTTPRequest request, final Track track)
 		return routeResult;
 	}
 
-	public String buildPatternBasedHashString(final DeliveryService deliveryService, final String requestPath) {
+	public String buildPatternBasedHashString(final DeliveryService deliveryService, final HTTPRequest request) {
+		final String requestPath = request.getPath();
 		if (deliveryService.getConsistentHashRegex() != null && !deliveryService.getConsistentHashRegex().isEmpty() && !requestPath.isEmpty()) {
-			return buildPatternBasedHashString(deliveryService.getConsistentHashRegex(), requestPath);
+			final StringBuilder hashString = new StringBuilder(request.getPath());
+			for (String qkey : request.getQueryString().split("&")) {
+				qkey = qkey.split("=")[0];
+				if (deliveryService.consistentHashQueryParams.contains(qkey)) {
+					hashString.append(qkey);
 
 Review comment:
   I thought we needed to include the query param value in the hash string as well, not just the key?
   
   Also, we need to sort the query string key/values somehow so that a request for `/foo?one=1&two=2` is hashed exactly the same as `/foo?two=2&one=1`. Some unit tests to verify these behaviors would be good too.
   
   Also, how should we handle the case of `/foo?a=b&a=c` -- two keys with the same value? Just include both key/value pairs in the hash string? Later values overwrite earlier values? I'm thinking the former, which is along the lines of what this code is already doing.

----------------------------------------------------------------
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