Re: [PATCH v3 1/3] refs/reftable: introduce "reftable.lockTimeout"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 19, 2024, 21:34 UTC
- Message-ID
- <xmqqmsk35nvu.fsf@gitster.g>
- In-Reply-To
- <77cffd3b1eb638e05c031e2949fdc9374d599e05.1726653185.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> +reftable.lockTimeout:: > + Whenever the reftable backend appends a new table to the stack, it has > + to lock the central "tables.list" file before updating it. This config > + controls how long the process will wait to acquire the lock in case > + another process has already acquired it. Value 0 means not to retry at > + all; -1 means to try indefinitely. Default is 100 (i.e., retry for > + 100ms).
Existing timeout knobs are in a hierarchy that is too wide (i.e. core.*timeout) and this fixes the mistake by placing the name in a lot more appropriate name (i.e. reftable.*timeout). If I were designing the system from scratch, I would probably place all of them in "refs.*timeout", but I do not think it is worth extra engineering effort to rename them and pay the transition cost.
Show 15 quoted lines
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 1c4b19e737f..ca281e39a29 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -256,6 +256,13 @@ static int reftable_be_config(const char *var, const char *value,
> if (factor > UINT8_MAX)
> die("reftable geometric factor cannot exceed %u", (unsigned)UINT8_MAX);
> opts->auto_compaction_factor = factor;
> + } else if (!strcmp(var, "reftable.locktimeout")) {
> + int64_t lock_timeout = git_config_int64(var, value, ctx->kvi);
> + if (lock_timeout > LONG_MAX)
> + die("reftable lock timeout cannot exceed %"PRIdMAX, (intmax_t)LONG_MAX);
> + if (lock_timeout < 0 && lock_timeout != -1)
> + die("reftable lock timeout does not support negative values other than -1");
> + opts->lock_timeout_ms = lock_timeout;Existing lock timeouts this models after seems to consider a platform native "int" is good enough size to represent the timeout value in milliseconds, but the eventual user of this value in the lockfile API expects a long, so lock_timeout_ms being long is fine.
Perhaps #leftoverbits to straighten out the types used for the other two timeout configuration variables.