Re: [PATCH v5 1/3] reftable/stack: allow disabling of auto-compaction
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 8, 2024, 06:12 UTC
- Message-ID
- <ZhOKy5W5e11ce2Rd@tanuki>
- In-Reply-To
- <a7011dbc6aa5cb256957bda5456ca989ce75e4a5.1712255369.git.gitgitgadget@gmail.com>
On Thu, Apr 04, 2024 at 06:29:27PM +0000, Justin Tobler via GitGitGadget wrote:
Show 6 quoted lines
> From: Justin Tobler <jltobler@gmail.com> > > Move the `disable_auto_compact` option into `reftable_write_options` to > allow a stack to be configured with auto-compaction disabled. In a > subsequent commit, this is used to disable auto-compaction when a > specific environment variable is set.
This patch looks good to me. I think the commit subject and message could use a bit of polishing though: we do not add a new way to disable auto-compaction as that already exists. The important bit though is that this toggle is purely internal right now and thus cannot be accessed by library users.
I'd thus propose something along the following lines:
```
reftable/stack: expose option to disable auto-compaction While the reftable stack already has a bit controls whether or not
to run auto-compation, this bit is not accessible to users of the
library. There are usecases though where a caller may want to have
more control over auto-compaction. Move the `disable_auto_compact` option into `reftable_write_options`
to allow external callers to disable auto-compaction. This will be
used in a subsequent commit.
```Patrick
Show 107 quoted lines
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
> reftable/reftable-writer.h | 3 +++
> reftable/stack.c | 2 +-
> reftable/stack.h | 1 -
> reftable/stack_test.c | 11 ++++++-----
> 4 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
> index 7c7cae5f99b..155bf0bbe2a 100644
> --- a/reftable/reftable-writer.h
> +++ b/reftable/reftable-writer.h
> @@ -46,6 +46,9 @@ struct reftable_write_options {
> * is a single line, and add '\n' if missing.
> */
> unsigned exact_log_message : 1;
> +
> + /* boolean: Prevent auto-compaction of tables. */
> + unsigned disable_auto_compact : 1;
> };
>
> /* reftable_block_stats holds statistics for a single block type */
> diff --git a/reftable/stack.c b/reftable/stack.c
> index dde50b61d69..1a7cdad12c9 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -680,7 +680,7 @@ int reftable_addition_commit(struct reftable_addition *add)
> if (err)
> goto done;
>
> - if (!add->stack->disable_auto_compact) {
> + if (!add->stack->config.disable_auto_compact) {
> /*
> * Auto-compact the stack to keep the number of tables in
> * control. It is possible that a concurrent writer is already
> diff --git a/reftable/stack.h b/reftable/stack.h
> index d919455669e..c862053025f 100644
> --- a/reftable/stack.h
> +++ b/reftable/stack.h
> @@ -19,7 +19,6 @@ struct reftable_stack {
> int list_fd;
>
> char *reftable_dir;
> - int disable_auto_compact;
>
> struct reftable_write_options config;
>
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 351e35bd86d..4fec823f14f 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -325,7 +325,7 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
> * we can ensure that we indeed honor this setting and have
> * better control over when exactly auto compaction runs.
> */
> - st->disable_auto_compact = i != n;
> + st->config.disable_auto_compact = i != n;
>
> err = reftable_stack_new_addition(&add, st);
> EXPECT_ERR(err);
> @@ -497,6 +497,7 @@ static void test_reftable_stack_add(void)
> struct reftable_write_options cfg = {
> .exact_log_message = 1,
> .default_permissions = 0660,
> + .disable_auto_compact = 1,
> };
> struct reftable_stack *st = NULL;
> char *dir = get_tmp_dir(__LINE__);
> @@ -508,7 +509,6 @@ static void test_reftable_stack_add(void)
>
> err = reftable_new_stack(&st, dir, cfg);
> EXPECT_ERR(err);
> - st->disable_auto_compact = 1;
>
> for (i = 0; i < N; i++) {
> char buf[256];
> @@ -935,7 +935,9 @@ static void test_empty_add(void)
>
> static void test_reftable_stack_auto_compaction(void)
> {
> - struct reftable_write_options cfg = { 0 };
> + struct reftable_write_options cfg = {
> + .disable_auto_compact = 1,
> + };
> struct reftable_stack *st = NULL;
> char *dir = get_tmp_dir(__LINE__);
>
> @@ -945,7 +947,6 @@ static void test_reftable_stack_auto_compaction(void)
> err = reftable_new_stack(&st, dir, cfg);
> EXPECT_ERR(err);
>
> - st->disable_auto_compact = 1; /* call manually below for coverage. */
> for (i = 0; i < N; i++) {
> char name[100];
> struct reftable_ref_record ref = {
> @@ -994,7 +995,7 @@ static void test_reftable_stack_add_performs_auto_compaction(void)
> * we can ensure that we indeed honor this setting and have
> * better control over when exactly auto compaction runs.
> */
> - st->disable_auto_compact = i != n;
> + st->config.disable_auto_compact = i != n;
>
> strbuf_reset(&refname);
> strbuf_addf(&refname, "branch-%04d", i);
> --
> gitgitgadget
>