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