Re: [PATCH v2 0/3] reftable/stack: use geometric table compaction

From: Han-Wen Nienhuys <hanwenn@gmail.com>
Date: Apr 3, 2024, 10:13 UTC
Message-ID: <CAOw_e7az+S4nkG1HKDcqXkK0BOhs6cBpkb=QEGBoZW8MSEEvGg@mail.gmail.com>
In-Reply-To: <Zfzd8YdAwzYRYkA3@tanuki>

On Fri, Mar 22, 2024 at 10:51 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 5 quoted lines

> > Thanks for taking a look!
>
> Cc'ing Han-Wen and Josh for additional input. From my point of view the
> new algorithm is simpler to understand and less fragile, but I do wonder
> whether there is anything that we're missing.

Good spotting. I hadn't thought about alternating tables.

I have one minor criticism:

Environment variables are untyped global variables without any form of data protection, so I find them unsavoury, and have tried to avoid them throughout. (The whole reftable library only looks at $TMPDIR in tests). They're also accessible to end users, so it can become a feature that can inadvertently become a maintenance burden.

For testing, there is a stack->disable_auto_compact.

If you want to keep that style, I would elevate disable_auto_compact into reftable_write_options to make it API surface. This will let you use it in tests written in C, which can be unittests and therefore more precise and fine-grained. They also run more quickly, and are easier to instrument with asan/valgrind/etc. The test for tables with alternating sizes can be easily written in C.

If you really need it, you could initialize disable_auto_compact from the environment, but I would suggest avoiding it if possible.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen