On Mon, 01 Dec 2008 02:55:15 -0000
[EMAIL PROTECTED] wrote:
> Author: pquerna
> Date: Sun Nov 30 18:55:14 2008
> New Revision: 721952
>
> URL: http://svn.apache.org/viewvc?rev=721952&view=rev
> Log:
> Add two new modules, originally written at Joost, to handle load balancing
> across
> multiple apache servers within the same datacenter.
>
> mod_heartbeat generates multicast status messages with the current number of
> clients connected, but the formated can easily be extended to include other
> things.
>
> mod_heartmonitor collects these messages into a static file, which then can
> be
> used for other modules to make load balancing decisions on.
>
> Added:
> httpd/httpd/trunk/modules/cluster/ (with props)
> httpd/httpd/trunk/modules/cluster/Makefile.in (with props)
> httpd/httpd/trunk/modules/cluster/README.heartbeat
> httpd/httpd/trunk/modules/cluster/README.heartmonitor
> httpd/httpd/trunk/modules/cluster/config.m4
> httpd/httpd/trunk/modules/cluster/mod_heartbeat.c (with props)
> httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (with props)
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/README
>
[cut]
> Added: httpd/httpd/trunk/modules/cluster/mod_heartbeat.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartbeat.c?rev=721952&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/cluster/mod_heartbeat.c (added)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartbeat.c Sun Nov 30 18:55:14 2008
> @@ -0,0 +1,354 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
[cut]
> +typedef struct hb_ctx_t
> +{
> + int active;
> + apr_sockaddr_t *mcast_addr;
> + int server_limit;
> + int thread_limit;
> + int status;
[cut]
> +
> +static void *hb_worker(apr_thread_t *thd, void *data)
Don't this need to be APR_THREAD_FUNC?
> +{
> + hb_ctx_t *ctx = (hb_ctx_t *) data;
> + apr_status_t rv;
> +
> + apr_pool_t *pool = apr_thread_pool_get(thd);
> + apr_pool_tag(pool, "heartbeat_worker");
> + ctx->status = 0;
The meaning of "status zero" is unclear.
[cut]
> +static void start_hb_worker(apr_pool_t *p, hb_ctx_t *ctx)
> +{
> + apr_status_t rv;
> +
> + rv = apr_thread_mutex_create(&ctx->start_mtx, APR_THREAD_MUTEX_UNNESTED,
> + p);
> +
> + if (rv) {
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> + "Heartbeat: apr_thread_cond_create failed");
> + ctx->status = rv;
> + return;
> + }
status is apr_status_t?
> +
> + apr_thread_mutex_lock(ctx->start_mtx);
> +
> + apr_pool_cleanup_register(p, ctx, hb_pool_cleanup,
> apr_pool_cleanup_null);
> +
> + rv = apr_thread_create(&ctx->thread, NULL, hb_worker, ctx, p);
> + if (rv) {
> + apr_pool_cleanup_kill(p, ctx, hb_pool_cleanup);
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> + "Heartbeat: apr_thread_create failed");
> + ctx->status = rv;
> + }
Same above.
> +
> + apr_thread_mutex_lock(ctx->start_mtx);
> + apr_thread_mutex_unlock(ctx->start_mtx);
> + apr_thread_mutex_destroy(ctx->start_mtx);
> +}
> +
> +static void hb_child_init(apr_pool_t *p, server_rec *s)
> +{
> + hb_ctx_t *ctx = ap_get_module_config(s->module_config,
> &heartbeat_module);
> +
> + apr_proc_mutex_child_init(&ctx->mutex, ctx->mutex_path, p);
> +
> + ctx->status = -1;
I don't like this. "status -1" is unclear.
> +
> + if (ctx->active) {
> + start_hb_worker(p, ctx);
> + if (ctx->status != 0) {
Same above.
Why not change the type of hb_ctx_t::status to apr_status_t ?
[cut]
> +static const char *cmd_hb_address(cmd_parms *cmd,
> + void *dconf, const char *addr)
> +{
> + apr_status_t rv;
> + char *host_str;
> + char *scope_id;
> + apr_port_t port = 0;
> + apr_pool_t *p = cmd->pool;
> + hb_ctx_t *ctx =
> + (hb_ctx_t *) ap_get_module_config(cmd->server->module_config,
> + &heartbeat_module);
> + const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> + if (err != NULL) {
> + return err;
> + }
> +
> + ctx->active = 1;
> +
> + rv = apr_parse_addr_port(&host_str, &scope_id, &port, addr, p);
cmd->temp_pool is better than cmd->pool.
> +
> + if (rv) {
> + return "HeartbeatAddress: Unable to parse address.";
> + }
> +
> + if (host_str == NULL) {
> + return "HeartbeatAddress: No host provided in address";
> + }
> +
> + if (port == 0) {
> + return "HeartbeatAddress: No port provided in address";
> + }
> +
> + rv = apr_sockaddr_info_get(&ctx->mcast_addr, host_str, APR_INET, port, 0,
> + p);
> +
> + if (rv) {
> + return "HeartbeatAddress: apr_sockaddr_info_get failed.";
> + }
> +
> + const char *tmpdir = NULL;
> + rv = apr_temp_dir_get(&tmpdir, p);
Same above.
> + if (rv) {
> + return "HeartbeatAddress: unable to find temp directory.";
> + }
> +
> + char *path = apr_pstrcat(p, tmpdir, "/hb-tmp.XXXXXX", NULL);
Same above.
[cut]
> Added: httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c?rev=721952&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (added)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c Sun Nov 30 18:55:14
> 2008
> @@ -0,0 +1,551 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements. See the NOTICE file distributed with
[cut]
> +typedef struct hm_ctx_t
> +{
> + int active;
> + const char *storage_path;
> + apr_proc_mutex_t *mutex;
> + const char *mutex_path;
> + apr_sockaddr_t *mcast_addr;
> + int status;
Similar to mod_heartbeat.c
> + int keep_running;
> + apr_thread_mutex_t *start_mtx;
> + apr_thread_t *thread;
> + apr_socket_t *sock;
> + apr_pool_t *p;
> + apr_hash_t *servers;
> +} hm_ctx_t;
> +
[cut]
> +static void *hm_worker(apr_thread_t *thd, void *data)
Don't this need to be APR_THREAD_FUNC?
> +{
> + hm_ctx_t *ctx = (hm_ctx_t *) data;
> + apr_status_t rv;
> +
> + ctx->p = apr_thread_pool_get(thd);
> + ctx->status = 0;
> + ctx->keep_running = 1;
> + apr_thread_mutex_unlock(ctx->start_mtx);
> +
> + while (ctx->keep_running) {
> + rv = apr_proc_mutex_trylock(ctx->mutex);
> + if (rv == APR_SUCCESS) {
> + break;
> + }
> + apr_sleep(apr_time_from_msec(200));
> + }
> +
> + rv = hm_listen(ctx);
> +
> + if (rv) {
> + ctx->status = rv;
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> + "Heartmonitor: Unable to listen for connections!");
> + apr_proc_mutex_unlock(ctx->mutex);
> + apr_thread_exit(ctx->thread, rv);
> + return NULL;
> + }
> +
> +
> + apr_time_t last = apr_time_now();
> + while (ctx->keep_running) {
> + int n;
> + apr_pool_t *p;
> + apr_pollfd_t pfd;
> + apr_interval_time_t timeout;
> + apr_pool_create(&p, ctx->p);
> +
> + apr_time_t now = apr_time_now();
> +
> + if (apr_time_sec((now - last)) > 5) {
> + hm_update_stats(ctx, p);
> + apr_pool_clear(p);
> + last = now;
> + }
> +
> + pfd.desc_type = APR_POLL_SOCKET;
> + pfd.desc.s = ctx->sock;
> + pfd.p = p;
> + pfd.reqevents = APR_POLLIN;
> +
> + timeout = apr_time_from_sec(1);
> +
> + rv = apr_poll(&pfd, 1, &n, timeout);
> +
> + if (!ctx->keep_running) {
> + break;
> + }
The parent of p is ctx->p, which is child's process pool,
so this is not a memory leak.
But to be clear IMHO it should call apr_pool_destroy.
Is it extra care?
[cut]
> +static const char *cmd_hm_storage(cmd_parms *cmd,
> + void *dconf, const char *path)
> +{
> + apr_pool_t *p = cmd->pool;
> + hm_ctx_t *ctx =
> + (hm_ctx_t *) ap_get_module_config(cmd->server->module_config,
> + &heartmonitor_module);
> + const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> + if (err != NULL) {
> + return err;
> + }
> +
> + ctx->storage_path = ap_server_root_relative(p, path);
> + ctx->mutex_path =
> + ap_server_root_relative(p, apr_pstrcat(p, path, ".hm-lock", NULL));
cmd->temp_pool is better than "p" for apr_pstrcat.
> +
> + return NULL;
> +}
> +
> +static const char *cmd_hm_listen(cmd_parms *cmd,
> + void *dconf, const char *mcast_addr)
> +{
> + apr_status_t rv;
> + char *host_str;
> + char *scope_id;
> + apr_port_t port = 0;
> + apr_pool_t *p = cmd->pool;
> + hm_ctx_t *ctx =
> + (hm_ctx_t *) ap_get_module_config(cmd->server->module_config,
> + &heartmonitor_module);
> + const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> + if (err != NULL) {
> + return err;
> + }
> +
> + ctx->active = 1;
> +
> + rv = apr_parse_addr_port(&host_str, &scope_id, &port, mcast_addr, p);
cmd->temp_pool is better than "p".
Regards,
Takashi
--
Takashi Sato <[EMAIL PROTECTED]>
