On Wed, Mar 20, 2024 at 03:22:55PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
