Martin,

On 6/22/17 3:04 AM, Martin Grigorov wrote:
> On Wed, Jun 21, 2017 at 9:05 PM, <schu...@apache.org> wrote:
>
>> +    /**
>> +     * The request attribute key where the load-balancer's activation
>> state
>> +     * can be found.
>> +     */
>> +    static final String ATTRIBUTE_KEY_JK_LB_ACTIVATION =
>> "JK_LB_ACTIVATION";
>>
> > Any objection to make this constant public and visible from outside
> ? I find it useful to be able to refer the constant by name than its
> value when integrating.

No objections. It looks like I'll have a follow-on patch shortly, so I
can change this at the same time.

>> +            // Kill any session cookie present
>> +            if(null != cookies) {
>> +                for(Cookie cookie : cookies) {
>> +                    final String cookieName = cookie.getName();
>> +                    if(containerLog.isTraceEnabled())
>> +                        containerLog.trace("Checking cookie " +
>> cookieName + "=" + cookie.getValue());
>> +
>> +                    if(sessionCookieName.equals(cookieName)
>> +                       &&
request.getRequestedSessionId().equals(cookie.getValue()))
>> {
>> +                        sessionCookie = cookie;
>>
>
> Is it a good idea to 'break' here ?
> Do you expect more than one session cookies ?!

No, but I do expect that there may be more interesting cookies later on
in the list...

>> +                    } else
>> +                    // Is the client presenting a valid ignore-cookie
>> value?
>> +                    if(null != _ignoreCookieName
>> +                            && _ignoreCookieName.equals(cookieName)
>> +                            && null != _ignoreCookieValue
>> +                            &&
_ignoreCookieValue.equals(cookie.getValue()))
>> {
>> +                        ignoreRebalance = true;
>> +                    }
>> +                }

Like here ^^^^^.

>> +            // Re-write the URI if it contains a ;jsessionid parameter
>> +            String uri = request.getRequestURI();
>> +            String sessionURIParamName = "jsessionid";
>> +            SessionConfig.getSessionUriParamName(request.getContext());
>>
>
> It seems this bug has been inroduced during testing/debugging.
> The return value of
> "SessionConfig.getSessionUriParamName(request.getContext());"
> is ignored and the sessionURIParamName is always "jsessionid".

+1

I'll get that fixed. This Valve is a port of a Filter that I wrote
earlier and evidently that got lost in the shuffle.

Thanks for the review.

-chris

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to