Re: svn commit: r759386 - in /httpd/httpd/trunk/modules/proxy: mod_serf.c mod_serf.h

[Paul Querna <pa...@querna.org>]

On Sun, Mar 29, 2009 at 11:05 AM, Ruediger Pluem <rp...@apache.org> wrote:
> On 03/28/2009 12:10 AM, pquerna@apache.org wrote:
>> Author: pquerna
>> Date: Fri Mar 27 23:10:21 2009
>> New Revision: 759386
>>
>> URL: http://svn.apache.org/viewvc?rev=759386&view=rev
....
>> +static void * merge_server_config(apr_pool_t *p, void *basev, void *overridesv)
>> +{
>> +    serf_server_config_t *ctx = apr_pcalloc(p, sizeof(serf_server_config_t));
>> +    serf_server_config_t *base = (serf_server_config_t *) basev;
>> +    serf_server_config_t *overrides = (serf_server_config_t *) overridesv;
>> +
>> +    ctx->clusters = apr_hash_overlay(p, base->clusters, overrides->clusters);
>> +    return ctx;
>> +}
>
>
> Why do we need merging when the directive is global only?

I intend to change it to not be global only....  There is no reason
SerfCluster can't be server/vhost scoped.

>> +
>>  static const command_rec serf_cmds[] =
>>  {
>> -    AP_INIT_TAKE1("SerfPass", add_pass, NULL, OR_INDEXES/*making shit up*/,
>> -     "A prefix and destination"),
>> +    AP_INIT_TAKE_ARGV("SerfCluster", add_cluster, NULL, RSRC_CONF,
>> +                      "Configure a cluster backend"),
>> +    AP_INIT_TAKE1("SerfPass", add_pass, NULL, OR_INDEXES,
>> +                  "URL to reverse proxy to"),
>>      {NULL}
>>  };
>>
>> +typedef struct hb_table_baton_t {
>> +    apr_pool_t *p;
>> +    const char *msg;
>> +} hb_table_baton_t;
>> +
>> +static int hb_table_check(void *rec, const char *key, const char *value)
>> +{
>> +    hb_table_baton_t *b = (hb_table_baton_t*)rec;
>> +    if (strcmp(key, "path") != 0) {
>> +        b->msg = apr_psprintf(b->p,
>> +                              "SerfCluster Heartbeat Invalid parameter '%s'",
>> +                              key);
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static const char* hb_config_check(void *baton,
>> +                                   cmd_parms *cmd,
>> +                                   apr_table_t *params)
>> +{
>> +    hb_table_baton_t b;
>> +
>> +    if (apr_is_empty_table(params)) {
>> +        return "SerfCluster Heartbeat requires a path to the heartbat information.";
>> +    }
>> +
>> +    b.p = cmd->pool;
>> +    b.msg = NULL;
>> +
>> +    apr_table_do(hb_table_check, &b, params, NULL);
>> +
>> +    if (b.msg) {
>> +        return b.msg;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +typedef struct hb_server_t {
>> +    const char *ip;
>> +    int busy;
>> +    int ready;
>> +    int seen;
>> +} hb_server_t;
>> +
>> +static void
>> +argstr_to_table(apr_pool_t *p, char *str, apr_table_t *parms)
>> +{
>> +    char *key;
>> +    char *value;
>> +    char *strtok_state;
>> +
>> +    key = apr_strtok(str, "&", &strtok_state);
>> +    while (key) {
>> +        value = strchr(key, '=');
>> +        if (value) {
>> +            *value = '\0';      /* Split the string in two */
>> +            value++;            /* Skip passed the = */
>> +        }
>> +        else {
>> +            value = "1";
>> +        }
>> +        ap_unescape_url(key);
>> +        ap_unescape_url(value);
>> +        apr_table_set(parms, key, value);
>> +        key = apr_strtok(NULL, "&", &strtok_state);
>> +    }
>> +}
>> +
>> +static apr_status_t read_heartbeats(const char *path,
>> +                                    apr_array_header_t *servers,
>> +                                    apr_pool_t *pool)
>> +{
>> +    apr_finfo_t fi;
>> +    apr_status_t rv;
>> +    apr_file_t *fp;
>> +
>> +    if (!path) {
>> +        return APR_SUCCESS;
>> +    }
>> +
>> +    rv = apr_file_open(&fp, path, APR_READ|APR_BINARY|APR_BUFFERED,
>> +                       APR_OS_DEFAULT, pool);
>> +
>> +    if (rv) {
>> +        return rv;
>> +    }
>> +
>> +    rv = apr_file_info_get(&fi, APR_FINFO_SIZE, fp);
>> +
>> +    if (rv) {
>> +        return rv;
>> +    }
>> +
>> +    {
>> +        char *t;
>> +        int lineno = 0;
>> +        apr_table_t *hbt = apr_table_make(pool, 10);
>> +        char buf[4096];
>> +
>> +        while (apr_file_gets(buf, sizeof(buf), fp) == APR_SUCCESS) {
>> +            hb_server_t *server;
>> +            const char *ip;
>> +            lineno++;
>> +
>> +            /* comment */
>> +            if (buf[0] == '#') {
>> +                continue;
>> +            }
>> +
>> +
>> +            /* line format: <IP> <query_string>\n */
>> +            t = strchr(buf, ' ');
>> +            if (!t) {
>> +                continue;
>> +            }
>> +
>> +            ip = apr_pstrndup(pool, buf, t - buf);
>> +            t++;
>> +            server = apr_pcalloc(pool, sizeof(hb_server_t));
>> +            server->ip = ip;
>> +            server->seen = -1;
>> +            apr_table_clear(hbt);
>> +
>> +            argstr_to_table(pool, apr_pstrdup(pool, t), hbt);
>> +
>> +            if (apr_table_get(hbt, "busy")) {
>> +                server->busy = atoi(apr_table_get(hbt, "busy"));
>> +            }
>> +
>> +            if (apr_table_get(hbt, "ready")) {
>> +                server->ready = atoi(apr_table_get(hbt, "ready"));
>> +            }
>> +
>> +            if (apr_table_get(hbt, "lastseen")) {
>> +                server->seen = atoi(apr_table_get(hbt, "lastseen"));
>> +            }
>> +
>> +            if (server->busy == 0 && server->ready != 0) {
>> +                /* Server has zero threads active, but lots of them ready,
>> +                 * it likely just started up, so lets /4 the number ready,
>> +                 * to prevent us from completely flooding it with all new
>> +                 * requests.
>> +                 */
>> +                server->ready = server->ready / 4;
>
> Nitpick: server->ready = server->ready >> 2 seems to be better.
>
>> +            }
>> +
>> +            APR_ARRAY_PUSH(servers, hb_server_t *) = server;
>> +        }
>> +    }
>> +
>> +    return APR_SUCCESS;
>> +}
>> +
>> +static int hb_server_sort(const void *a_, const void *b_)
>> +{
>> +    hb_server_t *a = (hb_server_t*)a;
>> +    hb_server_t *b = (hb_server_t*)b;
>
> Guess that should be a_ and b_ above.
>

oh duh. fixed in r759673.

>> +    if (a->ready == b->ready) {
>> +        return 0;
>> +    }
>> +    else if (a->ready > b->ready) {
>> +        return -1;
>> +    }
>> +    else {
>> +        return 1;
>> +    }
>> +}
>
>
> Regards
>
> Rüdiger

Thanks for reviewing,

Paul