On 5/2/2020 3:56 AM, Son Luong Ngoc wrote: > Hi Derrick, > > Sorry for another late reply on this RFC. > This time is a question on the multi-pack-index repack process. > > Why expire *before* repack and not after? > > I thought `core.multiPackIndex=true` would prevent old pack files from > being used thus expiring immediately after repack is safe? (on that > note, are users > required to set this config prior to running the job?) > > If expiring immediately after repack()+verify() is not safe, then should > we have a minimum allowed interval set? (although I would preferred to > make expire() safe) Suppose we run "repack" and then "expire". Suppose that there is a concurrent Git process that has a read handle on the multi-pack-index from before the repack. This multi-pack-index has a pack-file that was repacked by the "repack" command, and hence was expired and deleted by the "expire" command (because all of its objects are in the _new_ pack). However, when the Git process with the old multi-pack-index reads an object pointing to that pack-file, it finds that the pack does not exist when it tries to load it. Git is usually pretty resilient to these kinds of things, but it seemed prudent to be extra careful here. By spacing out these jobs across a time where we don't expect concurrent Git processes to hold a read handle on that old multi-pack-index (say, 24 hours) then this method is safe. In fact, Scalar ensures that no concurrent Git process is running at the start of the job [1], which means that no Git processes are _still_ running since the last job (but this does not stop concurrent processes from starting after that point and before the 'expire' command). [1] https://github.com/microsoft/scalar/blob/489764b815dfea32bec5cd4228f2157766012784/Scalar.Common/Maintenance/PackfileMaintenanceStep.cs#L106-L111 >> + >> + if (multi_pack_index_verify()) { >> + warning(_("multi-pack-index verify failed after expire")); >> + return rewrite_multi_pack_index(); >> + } >> + >> + if (multi_pack_index_repack()) { >> + error(_("multi-pack-index repack failed")); >> + return 1; >> + } > > Again, I just think the decision to include verify() inside repack() > made this part a bit inconsistent. You're right. It is a bit inconsistent. That should be fixed in the next version. Thanks, -Stolee
