From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeff Garzik <jeff@garzik.org>
Subject: Re: [Patch 5/7] tabled: Add replication daemon
Date: Sat, 14 Nov 2009 18:44:26 -0500
Message-ID: <4AFF40DA.5020505@garzik.org>
References: <20091113233605.673bfb9e@redhat.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Return-path: <hail-devel-owner@vger.kernel.org>
In-Reply-To: <20091113233605.673bfb9e@redhat.com>
Sender: hail-devel-owner@vger.kernel.org
List-ID: <hail-devel.vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"; format="flowed"
To: Pete Zaitcev <zaitcev@redhat.com>
Cc: Project Hail List <hail-devel@vger.kernel.org>

On 11/14/2009 01:36 AM, Pete Zaitcev wrote:
> +void rep_scan(void)
> +{
> +	struct cursor cur;
> +	struct db_obj_ent *obj;
> +	unsigned long kcnt;
> +	time_t start_time, t;
> +
> +	rep_retire();
> +
> +	start_time = time(NULL);
> +	if (debugging)
> +		applog(LOG_DEBUG, "key scan start time %lu", (long)start_time);
> +
> +	memset(&cur, 0, sizeof(struct cursor));	/* enough to construct */
> +	cur.db_env = tdb.env;
> +	cur.db_objs = tdb.objs;
> +
> +	kcnt = 0;
> +	for (;;) {
> +		if ((t = time(NULL))>= start_time + 2) {
> +			if (debugging)
> +				applog(LOG_DEBUG,
> +				       "db release at keys %lu seconds %lu",
> +				       kcnt, (long)t);
> +			rep_scan_close(&cur);
> +		}
> +
> +		if (rep_scan_get(&cur,&obj) != 0)
> +			break;
> +
> +		/* not needed for db4 with DB_NEXT, but eases our logic */
> +		if (rep_scan_parse(&cur, obj) != 0) {
> +			free(obj);
> +			continue;
> +		}
> +
> +		if (!GUINT32_FROM_LE(obj->flags)&  DB_OBJ_INLINE)
> +			rep_scan_verify(&cur, obj);
> +
> +		free(obj);
> +		kcnt++;
> +	}
> +
> +	rep_scan_close(&cur);
> +	free(cur.key);
> +	cur.key = NULL;
> +
> +	if (debugging)
> +		applog(LOG_DEBUG, "key scan done keys %lu", kcnt);
> +	return;

Major comments:

1) What is the point of db->del() in rep_add_nid() ?  You are in the 
middle of a transaction, and you immediately overwrite that record in 
the same transaction.  That is clearly unnecessary work, when db->put() 
will simply overwrite an existing record, if requested.


2) rep_scan():  I would rather not make the entire daemon non-responsive 
for multiple seconds.

The database is already in multi-threaded mode, and db4 is 
free-threaded, so it would seem to make a lot more sense to simply 
g_thread_create() a thread to do this work.

Also, there should be no need to scan a chunkd's keys at all, as long as 
the chunkd instance is still communicating with us.

	Jeff