Make the health check timeout for balancer member configurable #385
Conversation
|
Hello @ylavic, I really appreciate any insights or feedback you might have about this PR. |
|
Hello @icing, as this PR will be closed soon, I really appreciate any insights or feedback you might have about it. |
|
@Mrida sorry, I am not really familiar with that part of the code. I recommend contacting the dev mailing list to get more attention at dev@httpd.apache.org |
modules/proxy/mod_proxy_hcheck.c
Outdated
| @@ -42,8 +42,10 @@ typedef struct { | |||
| int passes; | |||
| int fails; | |||
| apr_interval_time_t interval; | |||
| apr_interval_time_t hc_timeout; | |||
There was a problem hiding this comment.
If a patch will be used in 2.4.x, you need to only append to structs
modules/proxy/mod_proxy.h
Outdated
| @@ -460,6 +460,7 @@ typedef struct { | |||
| * may be available while exceeding the soft limit */ | |||
| apr_interval_time_t retry; /* retry interval */ | |||
| apr_interval_time_t timeout; /* connection timeout */ | |||
| apr_interval_time_t hc_timeout; /* health check timeout */ | |||
There was a problem hiding this comment.
If a patch will be used in 2.4.x, you need to only append to structs
modules/proxy/mod_proxy.h
Outdated
| @@ -478,6 +479,7 @@ typedef struct { | |||
| unsigned int is_address_reusable:1; | |||
| unsigned int retry_set:1; | |||
| unsigned int timeout_set:1; | |||
| unsigned int hc_timeout_set:1; | |||
There was a problem hiding this comment.
If a patch will be used in 2.4.x, you need to only append to structs
|
Hello @trawick, I really appreciate any input you might have about this pr. |
| @@ -223,6 +229,19 @@ static const char *set_worker_hc_param(apr_pool_t *p, | |||
| PROXY_STRNCPY(worker->s->hcexpr, val); | |||
| } else { | |||
| temp->hcexpr = apr_pstrdup(p, val); | |||
| } | |||
| } else if(!strcasecmp(key, "hctimeout")) { | |||
There was a problem hiding this comment.
Code style (and above/below code) suggests that } else if () { should rather be:
}
else if (...) {
...
}
here.
| worker->s->hc_timeout_set = 1; | ||
| } else { | ||
| temp->hc_timeout = timeout; | ||
| temp->hc_timeout_set = 1; | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
And while at it (possibly) indent this misaligned else correctly.
| hc->s->ping_timeout = worker->s->ping_timeout; | ||
| hc->s->timeout_set = worker->s->timeout_set; | ||
| hc->s->timeout = worker->s->timeout; | ||
| } |
There was a problem hiding this comment.
If we don't have a specific hc for the other timeouts (conn_ and ping_), shouldn't we keep the worker's ones at least rather than the defaults?
| if (hc->s->ping_timeout_set) { | ||
| if (hc->s->hc_timeout_set) { | ||
| timeout = hc->s->hc_timeout; | ||
| } else if (hc->s->ping_timeout_set) { | ||
| timeout = hc->s->ping_timeout; |
There was a problem hiding this comment.
I understand that the hc_timeout is the only configurable one specifically for the healthcheck mechanism, but I'm not sure that it should prevail on the (AJP) worker's ping_timeout if a AJP ping is configured (it's usually short in this case anyway).
Maybe this change is not needed, or we should be able configure all the timeouts specifically for the healthcheck, but this could add more settings than necessary (i.e. never used, useless documentation..) so maybe we should wait for a real usecase?
Atfer httpd upgrade to version 55, the health check timeout has been set to be equal to the request timeout.
We have lost the capability to define a timeout for the health check that is different from that of the request.
Our worker request timeout is set to 60s (time needed to treat a request) while its health check should be quick (5s).
The need for a short health check is to allow httpd to quickly detect suspended workers (that do not receive hc) and set their health status to HcFl so that requests do not get routed to them.
The goal of this pull request is to make the timeout of the health check configurable by balancer member.