\n * This is generally a transient failure and is\nusually caused by\n * another process trying to access the ref at the\nsame time as this\n * process was trying to update it. It is possible a\nfuture operation\n * will be successful.\n */\n\n* reftable/stack: gracefully handle failed auto-compaction due to locks\n\nIt's a bit unsatisfying that you have to use details of the locking\nprotocol to test it, but I couldn't think of a way to unittest this\nusing only the API. Maybe it's worth considering removing the\nautomatic compaction from the reftable-stack.h API, and have the\ncaller (eg. in refs/reftable-backend.c) call it explicitly?\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"490934","messageId":"Zfm4-jCWV81VyAXt@framework","threadId":"61148","inReplyTo":"CAOw_e7aEPE1QRsqsgvdBVGkk2uFo4e080wWbM5dsVwkiSpYcbA@mail.gmail.com","subject":"Re: [PATCH 00/15] refs: introduce `--auto` to pack refs as needed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-19T16:10:34Z","receivedAt":"2024-03-19T16:10:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 18, 2024 at 11:14:58PM +0100, Han-Wen Nienhuys wrote:\n> I had a quick look over the reftable bits of this series. It looks OK,\n> but here are some comments. Nothing blocking.\n> \n> * reftable/error: discern locked/outdated errors\n> \n> It is not obvious to me why you need two different codes. Is it so you\n> can print the offending lock file (so people can delete them\n> manually?). FWIW, this was based on JGit, which has\n> \n> /**\n> * The ref could not be locked for update/delete.\n> *
\n> * This is generally a transient failure and is\n> usually caused by\n> * another process trying to access the ref at the\n> same time as this\n> * process was trying to update it. It is possible a\n> future operation\n> * will be successful.\n> */\n\nI just think that these are somewhat different failure modes. Not being\nable to lock a file because it already is locked is totally different\nthan realizing that files have been concurrently modified and are thus\nout of date now.\n\nSo the main motivator why I split up the error codes is that the error\nmessage will end up being shown to the user. Before we would say \"data\nis outdated\" for both cases. Now we either say \"data is locked\", or we\nsay \"data concurrently modified\", which I think is a lot more helpful to\nthe user.\n\n> * reftable/stack: gracefully handle failed auto-compaction due to locks\n> \n> It's a bit unsatisfying that you have to use details of the locking\n> protocol to test it, but I couldn't think of a way to unittest this\n> using only the API. Maybe it's worth considering removing the\n> automatic compaction from the reftable-stack.h API, and have the\n> caller (eg. in refs/reftable-backend.c) call it explicitly?\n\nI think it's not all that bad. Yes, we now need to know about error\ncodes returned by the auto-compaction code. But it's still contained in\nthe reftable library. If we were to remove it from the API itself then\nthis knowledge would need to exist in all callers of the auto compaction\ncode which live _outside_ of the library, which I think would be even\nworse.\n\nWe probably could add a unit test that basically does the same as the\nadded test in t0610. That is, we lock all tables in the stack and then\ncheck that `reftable_addition_commit()` fails gracefully when trying to\ncompact the already-locked tables.\n\nBut that actually highlights another issue that we will tackle soonish:\nI think that the compaction code should handle already-locked files more\ngracefully. E.g. when tables 0..N are locked by a concurrent process,\nthen the reftable library should realize that and only try to compact\ntables N+1..M. Justin Tobler is currently working on some refactorings\nof the auto-compaction logic in [1], so we will tackle this issue once\na revised version of his patch has landed.\n\n[1]: https://lore.kernel.org/git/pull.1683.git.1709669025722.gitgitgadget@gmail.com/\n\nPatrick\n"}]}