Re: [PATCH v2 2/7] maintenance: store the "last run" time in config

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 26, 2020, 13:34 UTC
Message-ID: <4fe41132-c8f5-6b85-414f-5a1e5fcccdc9@gmail.com>
In-Reply-To: <xmqqh7sqz93k.fsf@gitster.c.googlers.com>

On 8/25/2020 5:52 PM, Junio C Hamano wrote:

Show 12 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> I selected the timestamp before the task, as opposed to after the task,
>> for a couple reasons:
>>
>>  1. The time the task takes to execute should not contribute to the
>>     interval between running the tasks.
> 
> ... as long as the run time is sufficiently shorter than the
> interval, that is.  If a task takes 10-30 minutes depending on how
> dirty the repository is, it does not make sense to even try to run
> it every 15 minutes.

Definitely. The lock on the object database from earlier prevents these longer-than-anticipated tasks from stacking.

Show 6 quoted lines

>> If a daily task takes 10 minutes
>>     to run, then every day the execution will drift by at least 10
>>     minutes.
> 
> That is not incorrect per-se, but it does not tell us why drifting
> by 10 minutes is a bad thing.

True.

Show 19 quoted lines

>>  2. If the task fails for some unforseen reason, it would be good to
>>     indicate that we _attempted_ the task at a certain timestamp. This
>>     will avoid spamming a repository that is in a bad state.
> 
> Absolutely.
> 
>> +static void update_last_run(struct maintenance_task *task)
>> +{
>> +	timestamp_t now = approxidate("now");
>> +	struct strbuf config = STRBUF_INIT;
>> +	struct strbuf value = STRBUF_INIT;
>> +	strbuf_addf(&config, "maintenance.%s.lastrun", task->name);
>> +	strbuf_addf(&value, "%"PRItime"", now);
> 
> So is this essentially meant as a human-unreadable opaque value,
> like we have in the commit object header lines?  I do not have a
> strong opinion, but it would be nice to allow curious to casually
> read it.  Perhaps "git config --type=timestamp maintenance.lastrun"
> can be taught to pretty print its value?

Good idea. I will think on this. Of course, we already have --type=expiry-date, which does the opposite. Perhaps this config value should be a human-readable date and then be parsed into a timestamp in-process using git_config_expiry_date()?

I have mixed feelings on using that format, because it can store both a fixed or relative datetime. The *.lastRun config really wants a _fixed_ datetime, but the *.schedule config (in the next patch) would want a _relative_ datetime. This also allows things like "now" or "never", so it presents a lot of flexibility for users. A nightmare to test, but perhaps that flexibility is useful.

(Of course, in another thread you mentioned multiple `crontab` lines, which might make this entire discussion irrelevant. I'll follow up there.)

Thanks, -Stolee