Re: [PATCH 6/9] reftable/stack: return stack segments directly
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Oct 13, 2025, 09:01 UTC
- Message-ID
- <CAOLa=ZSfiD4KTSMyc2WW4oC2zyDO9CHmUWwnws3GhCdbtL4rAA@mail.gmail.com>
- In-Reply-To
- <aOjse9udPEoWqUaO@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote: >> The `stack_table_sizes_for_compaction()` function returns individual >> sizes of each reftable table. This function is only called by >> `reftable_stack_auto_compact()` to process the table sizes. > > Maybe: "to decide which tables need to be compacted, if any." >
Reads better, will update.
>> Modify the function to directly return the segments, this avoids the > > s/this/which/ >
will do.
Show 7 quoted lines
>> extra step of receiving the sizes only to pass it to >> `suggest_compaction_segment()`. >> >> A future commit will also add functionality for checking if > > s/if/whether/ >
This should be okay either ways I believe, but shouldn't matter much, will change.
Show 39 quoted lines
>
>> auto-compaction is necessary without performing it. This change allows
>> code re-usability in that context.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> reftable/stack.c | 23 ++++++++++++-----------
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/reftable/stack.c b/reftable/stack.c
>> index f91ce50bcd..9d9326ce0e 100644
>> --- a/reftable/stack.c
>> +++ b/reftable/stack.c
>> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
>>
>> REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
>> if (!sizes)
>> - return NULL;
>> + return REFTABLE_OUT_OF_MEMORY_ERROR;
>>
>> for (size_t i = 0; i < st->merged->tables_len; i++)
>> sizes[i] = st->tables[i]->size - overhead;
>>
>> - return sizes;
>> + *seg = suggest_compaction_segment(sizes, st->merged->tables_len,
>> + st->opts.auto_compaction_factor);
>> + reftable_free(sizes);
>> +
>> + return 0;
>> }
>>
>> int reftable_stack_auto_compact(struct reftable_stack *st)
>> {
>> struct segment seg;
>> - uint64_t *sizes;
>> + int err = 0;
>
> The initialization is unnecessary.
>True, but I always think its nicer to define the base state like this. This makes it less error prone when/if the code is modified in the future. But doesn't really matter, so let me remove it.
> Other than that this patch looks good to me. > > Patrick
Thanks for the review.