Re: [PATCH] reftable/stack: fsync "tables.list" during compaction

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 31, 2024, 05:46 UTC
Message-ID: <ZbnevPIN5cudLU5w@tanuki>
In-Reply-To: <CAGAWz+4wyd13UYRH3ZSSZEq1Y=5HE_p+qaXeOJS-ENMpWoLabA@mail.gmail.com>

On Tue, Jan 30, 2024 at 01:04:46PM -0600, Justin Tobler wrote:

Show 14 quoted lines

> On Mon, Jan 29, 2024 at 11:23 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added
> > code to fsync both newly written reftables as well as "tables.list" to
> > disk. But there are two code paths where "tables.list" is being written:
> >
> >   - When appending a new table due to a normal ref update.
> >
> >   - When compacting a range of tables during compaction.
> >
> > We have only addressed the former code path, but do not yet sync the new
> > "tables.list" file in the latter. Fix this ommission.
> 
> nit: s/ommission/omission

I knew this looked weird when writing it! Should've looked it up.

Show 6 quoted lines

> > Note that we are not yet adding any tests. These tests will be added
> > once the "reftable" backend has been upstreamed.
> >
> 
> Nice catch! I noticed a small typo in the commit message but otherwise looks
> good to me.

Thanks. I'll refrain from sending out a new version for this typo alone, but will fix it if other feedback requires a second iteration.

Patrick