https://bz.apache.org/bugzilla/show_bug.cgi?id=64338
--- Comment #4 from Christopher Schultz <[email protected]> --- (In reply to Ruediger Pluem from comment #3) > Thanks for the patch. Long-time listener. First-time patcher. :) > Some points: > > 1. Why not setting this environment variable in the same block as > BALANCER_WORKER_ROUTE (line 622) but only when a session route was supplied? This is my ignorance showing. I figured that if there was no route, then the request wouldn't actually be delivered anywhere and it would be wasted effort. What does the condition on line 632 actually check? /* Rewrite the url from 'balancer://url' * to the 'worker_scheme://worker_hostname[:worker_port]/url' * This replaces the balancers fictional name with the * real hostname of the elected worker. */ access_status = rewrite_url(r, *worker, url); /* Add the session route to request notes if present */ if (route) { // <=================== line 632 apr_table_setn(r->notes, "session-sticky", sticky); apr_table_setn(r->notes, "session-route", route); /* Add session info to env. */ apr_table_setn(r->subprocess_env, "BALANCER_SESSION_STICKY", sticky); apr_table_setn(r->subprocess_env, "BALANCER_SESSION_ROUTE", route); apr_table_setn(r->subprocess_env, "BALANCER_WORKER_STATUS", apr_itoa(r->pool, (*worker)->s->status)); I'm happy to move this to a more appropriate location. > 2. I haven't checked it we could run into issues as apr_itoa takes an int > while worker->s->status is an unsigned int. At least this could cause > compiler warnings. I got no compiler warnings using gcc, though I didn't specifically enable anything during the build. I just assumed warnings were enabled during compile. But I do take your point: status is unsigned int and I'm using atoi. All the current statuses are non-negative and don't run higher than 0x800 so we're safe for now, but it's definitely not future-proof. I didn't see apr_uitoa, but of course there is still apr_ltoa. I just wanted to be as minimally impactful on performance as possible. I suppose I could check the magnitude of 'status' and use apr_ltoa if necessary after a cast. What is your recommendation, here? > 3. Please add a documentation for your new variable to > docs/manual/mod/mod_proxy_balancer.xml at the section environment starting > at about line 130. Aha! I can certainly do that. -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
