Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 8, 2023, 13:56 UTC
Message-ID: <8aa7be24-f4a8-4515-8425-dd4bc69e07b1@github.com>
In-Reply-To: <adc0cf80-c2e3-7af2-6d7f-036b95d27f80@gmail.com>

On 8/8/2023 5:53 AM, Phillip Wood wrote:> Hi Stolee

Show 19 quoted lines

> 
> On 07/08/2023 19:51, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>
>> +    switch (schedule) {
>> +    case SCHEDULE_HOURLY:
>> +        schedule_pattern = xstrfmt("*-*-* *:%02d:00", minute);
>> +        break;
>> +
>> +    case SCHEDULE_DAILY:
>> +        schedule_pattern = xstrfmt("*-*-* 0:%02d:00", minute);
>> +        break;
>> +
>> +    case SCHEDULE_WEEKLY:
>> +        schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
>> +        break;
> 
> This is not a new issue with this patch but we run the hourly job even
> when we want to run the daily job or the weekly job and we run the daily
> job when we want to run the weekly job.

This is an excellent point! Thanks for bringing it up.

Show 7 quoted lines

> So only one of these jobs will succeed. The cron entries are careful to
> only run one job at a time, I think it would be worth doing the same
> thing here. I think the using the following format strings would fix this.
> 
> Hourly: "Tue..Sun *-*-* 1..23:00:%02d"
> Daily:  "Tue..Sun *-*-* 00:00:%02d"
> Weekly: "Mon      *-*-* 00:00:%02d"

I would modify this with dropping the "Tue..Sun" from the hourly schedule, as we still want 23 runs on Mondays.

> It looks like the launchctl schedule has the same issue.

I will take a look at this and consider some additional patches to correct these issues across both schedulers. Thank you for the attention to detail!

> One thing I've been wondering about which is related to maintenance but
> totally off-topic for this patch is that I think when auto maintenance
> is enabled we stop automatically running "gc" so how do the reflogs get
> expired?

The default maintenance schedule does not include a 'gc' run as it does not intend to remove any data. Reflog expiration could be considered as a separate maintenance task that might be useful in the default maintenance schedule.

Thanks, -Stolee