You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "Larry McCay (JIRA)" <ji...@apache.org> on 2017/07/07 15:56:00 UTC
[jira] [Comment Edited] (KNOX-843) Add support for load balancing
across HA instances
[ https://issues.apache.org/jira/browse/KNOX-843?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16078284#comment-16078284 ]
Larry McCay edited comment on KNOX-843 at 7/7/17 3:55 PM:
----------------------------------------------------------
Sorry for the delay in reviewing this patch, [~jeffreyr97].
It looks pretty good.
A couple things:
1. the javadoc/comment of this method in DefaultHaDispatch needs to be cleaned up and I'd like to understand the "ditch request" condition and where in the method it is actually ditching it - as typically it would be bad to ditch a request...
{code}
/*
service1 token1 HttpOnly cookie 60 sec Attach cookie with random value
service2 token2
service3 token3
service4 token4
if request has no cookie STICK then
get next service in round robin
send service inbound request
get service outbound request
attach cookie STICK with random value
return as knox response
else if request has cookie STICK
if derive service position based on STICK random value
send service inbound request
get service outbound request
return as knox response
else
ditch request
*/
private void balancerRequest(HttpUriRequest serviceHost, HttpServletRequest inboundRequest, HttpServletResponse outboundResponse ) throws IOException {
Cookie stickCookie = null;
HttpResponse inboundResponse = null;
if( (stickCookie = hasStickCookie( inboundRequest )) != null ) {
URI uri = null;
try {
uri = new URI( (String) stickCookie.getValue());
} catch (URISyntaxException e) {
throw new IOException();
}
((HttpRequestBase) serviceHost).setURI(getCurrentUri(inboundRequest));
inboundResponse = executeOutboundRequest(serviceHost);
writeOutboundResponse(serviceHost, inboundRequest, outboundResponse, inboundResponse);
} else {
addStickyCookie(inboundRequest, outboundResponse);
((HttpRequestBase) serviceHost).setURI(getCurrentUri(inboundRequest));
inboundResponse = executeOutboundRequest(serviceHost);
writeOutboundResponse(serviceHost, inboundRequest, outboundResponse, inboundResponse);
haProvider.changeActiveNextAvailable(getServiceRole());
}
}
{code}
2. I don't like the name of the method. It seems to me that we are asking for the request to be balanced. I think we should just change it to balanceRequest as that will be aligned with the others executeRequest, failoverRequest by being a verb rather than what seems more like a noun.
3. the following method is looking for a hardcoded cookie name "STICK" but the constant org.apache.hadoop.gateway.ha.dispatch.DefaultHaDispatch.STICKY_COOKIE_NAME is "STICKY". Do these represent different things or are they the same and there is a typo here?
{code}
private Cookie hasStickCookie(HttpServletRequest inboundRequest ){
Cookie[] cookies = inboundRequest.getCookies();
Cookie stickCookie = null;
if (cookies != null){
for (int i = 0; i < cookies.length; i++){
if (cookies[i].getName( ).equals("STICK")){
stickCookie = cookies[i];
}
}
}
return stickCookie;
}
{code}
4. Also, in the above method, it seems to assume that there may be multiple cookies with the same Name and will accept the last one. (I notice that WebSSOResource has the same problem actually). Is there any reason to do this or should you just accept the first and break the loop?
5. It should also be noted that this loadbalancing isn't going to work properly if SSL is disabled in the gateway since the STICKY cookie is always set to secure only.
6. Shouldn't stickiness only b required for loadbalancing of services that require it due to some session state? Wouldn't be better loadbalancing for those that don't require it if they could always round robin.
was (Author: lmccay):
Sorry for the delay in reviewing this patch, [~jeffreyr97].
It looks pretty good.
A couple things:
1. the javadoc/comment of this method in DefaultHaDispatch needs to be cleaned up and I'd like to understand the "ditch request" condition and where in the method it is actually ditching it - as typically it would be bad to ditch a request...
/*
service1 token1 HttpOnly cookie 60 sec Attach cookie with random value
service2 token2
service3 token3
service4 token4
if request has no cookie STICK then
get next service in round robin
send service inbound request
get service outbound request
attach cookie STICK with random value
return as knox response
else if request has cookie STICK
if derive service position based on STICK random value
send service inbound request
get service outbound request
return as knox response
else
ditch request
*/
private void balancerRequest(HttpUriRequest serviceHost, HttpServletRequest inboundRequest, HttpServletResponse outboundResponse ) throws IOException {
Cookie stickCookie = null;
HttpResponse inboundResponse = null;
if( (stickCookie = hasStickCookie( inboundRequest )) != null ) {
URI uri = null;
try {
uri = new URI( (String) stickCookie.getValue());
} catch (URISyntaxException e) {
throw new IOException();
}
((HttpRequestBase) serviceHost).setURI(getCurrentUri(inboundRequest));
inboundResponse = executeOutboundRequest(serviceHost);
writeOutboundResponse(serviceHost, inboundRequest, outboundResponse, inboundResponse);
} else {
addStickyCookie(inboundRequest, outboundResponse);
((HttpRequestBase) serviceHost).setURI(getCurrentUri(inboundRequest));
inboundResponse = executeOutboundRequest(serviceHost);
writeOutboundResponse(serviceHost, inboundRequest, outboundResponse, inboundResponse);
haProvider.changeActiveNextAvailable(getServiceRole());
}
}
2. I don't like the name of the method. It seems to me that we are asking for the request to be balanced. I think we should just change it to balanceRequest as that will be aligned with the others executeRequest, failoverRequest by being a verb rather than what seems more like a noun.
3. the following method is looking for a hardcoded cookie name "STICK" but the constant org.apache.hadoop.gateway.ha.dispatch.DefaultHaDispatch.STICKY_COOKIE_NAME is "STICKY". Do these represent different things or are they the same and there is a typo here?
private Cookie hasStickCookie(HttpServletRequest inboundRequest ){
Cookie[] cookies = inboundRequest.getCookies();
Cookie stickCookie = null;
if (cookies != null){
for (int i = 0; i < cookies.length; i++){
if (cookies[i].getName( ).equals("STICK")){
stickCookie = cookies[i];
}
}
}
return stickCookie;
}
4. Also, in the above method, it seems to assume that there may be multiple cookies with the same Name and will accept the last one. (I notice that WebSSOResource has the same problem actually). Is there any reason to do this or should you just accept the first and break the loop?
5. It should also be noted that this loadbalancing isn't going to work properly if SSL is disabled in the gateway since the STICKY cookie is always set to secure only.
6. Shouldn't stickiness only b required for loadbalancing of services that require it due to some session state? Wouldn't be better loadbalancing for those that don't require it if they could always round robin.
> Add support for load balancing across HA instances
> --------------------------------------------------
>
> Key: KNOX-843
> URL: https://issues.apache.org/jira/browse/KNOX-843
> Project: Apache Knox
> Issue Type: New Feature
> Components: Server
> Affects Versions: 0.11.0
> Reporter: Sam Hjelmfelt
> Assignee: Jeffrey E Rodriguez
> Fix For: 0.13.0
>
> Attachments: KNOX-843.001.patch, KNOX-843.002.patch, KNOX-843.patch, KNOX-843.pdf
>
>
> Knox should support load balancing across HA service instances rather than just offering failover.
> One solution may be to add an option to the HAProvider.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)