Re: [PATCH 04/15] reftable/stack: gracefully handle failed auto-compaction due to locks

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

On Wed, Mar 20, 2024 at 03:22:55PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Whenever we commit a new table to the reftable stack we will end up
> > invoking auto-compaction of the stack to keep the total number of tables
> > at bay. This auto-compaction may fail though in case at least one of the
> > tables which we are about to compact is locked. This is indicated by the
> > compaction function returning a positive value. We do not handle this
> 
> We no longer return a positive value, do we?

Yup, this is stale.

> > case though, and thus bubble that return value up the calling chain,
> > which will ultimately cause a failure.
> >
> > Fix this bug by handling positive return values.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  reftable/stack.c           | 13 ++++++++++++-
> >  t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index ba15c48ddd..dd5160d751 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)
> >  	if (err)
> >  		goto done;
> >
> > -	if (!add->stack->disable_auto_compact)
> > +	if (!add->stack->disable_auto_compact) {
> > +		/*
> > +		 * Auto-compact the stack to keep the number of tables in
> > +		 * control. Note that we explicitly ignore locking issues which
> > +		 * may indicate that a concurrent process is already trying to
> > +		 * compact tables. This is fine, so we simply ignore that error
> > +		 * condition.
> > +		 */
> >
> 
> Nit: The last two sentences are somewhat the same, it'd be perhaps nicer
> to explain why "this is fine".

Fair enough.

> >  		err = reftable_stack_auto_compact(add->stack);
> > +		if (err < 0 && err != REFTABLE_LOCK_ERROR)
> > +			goto done;
> > +		err = 0;
> > +	}
> >
> >  done:
> >  	reftable_addition_close(add);
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > index 686781192e..5f2f9baa9b 100755
> > --- a/t/t0610-reftable-basics.sh
> > +++ b/t/t0610-reftable-basics.sh
> > @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '
> >  	EOF
> >  '
> >
> > +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +
> > +		test_commit A &&
> > +		for i in $(test_seq 10)
> > +		do
> > +			git branch branch-$i &&
> > +			for table in .git/reftable/*.ref
> > +			do
> > +				touch "$table.lock" || exit 1
> > +			done ||
> > +			exit 1
> > +		done &&
> > +		test_line_count = 13 .git/reftable/tables.list
> > +	)
> > +'
> 
> I'm not sure about the testing setup for reftables, but maybe if we
> moved this to the unit tests, we could've also checked the
> `reftable_stack_compaction_stats(st)->failures` value. This is fine, but
> it doesn't really tell us if the compaction was even attempted.

Both tests have their merit, I'd say. Let's thus just add both.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]