Raise errors in gc worker #2065
Conversation
icristescu
force-pushed
the
result_worker
branch
from
24aee6c
to
86f0dc3
Compare
Codecov Report
@@ Coverage Diff @@
## main #2065 +/- ##
==========================================
+ Coverage 64.34% 64.38% +0.03%
==========================================
Files 131 132 +1
Lines 15584 15570 -14
==========================================
- Hits 10028 10024 -4
+ Misses 5556 5546 -10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/irmin-pack/unix/gc.ml
Outdated
| Errors.finalise (fun _outcome -> | ||
| Ao.close prefix |> Errs.log_if_error "GC: Close prefix") | ||
| Ao.close_exn prefix ~log_if_error:"GC: Close prefix") |
There was a problem hiding this comment.
Since the callback for Errors.finalise has type 'a -> unit (and so the result monad doesn't really leave this scope), I'd say it is okay to leave as it is currently. And then you don't need Ao.close_exn.
There was a problem hiding this comment.
Yes agree, thanks!
src/irmin-pack/unix/gc.ml
Outdated
| in | ||
| Ok () | ||
| Errors.finalise (fun _outcome : unit -> | ||
| Io.fsync_and_close prefix |
There was a problem hiding this comment.
Same comment here which means no need for (Io|Ao).fsync_and_close.
| module Ao = struct | ||
| include Append_only_file.Make (Fm.Io) | ||
|
|
||
| let create_rw_exn ~path ~auto_flush_callback = |
There was a problem hiding this comment.
Thoughts on moving to Append_only_file? With other changes mentioned this would remove the need for this Ao extension.
There was a problem hiding this comment.
I'm tempted to leave it here. My understanding is that we don't want to propose a create_exn for other uses that in this part of the code. (as a minor plus, having it here, we can provide the defaults for overwrite and auto_flush_threshold).
Agree that this module wrapping is a bit ugly, so I can remove it and unwrap the function when used, if you prefer.
There was a problem hiding this comment.
create_rw_exn would be the only exception-based function in Append_only_file that exists for convenience reasons. The others exist for performance reasons.
My taste would be to leave create_rw_exn here, but that's only my taste
There was a problem hiding this comment.
Those are all good reasons to keep it here. 👍
src/irmin-pack/unix/gc.ml
Outdated
| let open Result_syntax in | ||
| fsync x >>= (fun () -> close x) |> Errs.log_if_error log_if_error | ||
|
|
||
| let open_exn ~path ~readonly = |
There was a problem hiding this comment.
Thoughts on moving to Io? With other changes mentioned this would remove the need for this Io extension.
| in | ||
| Ok suffix | ||
| let auto_flush_callback = Ao.flush_exn in | ||
| Ao.create_rw_exn ~path ~auto_flush_callback |
src/irmin-pack/unix/gc.ml
Outdated
| let* () = Mapping_file.iter mapping f in | ||
| Ao.flush prefix | ||
| let () = Mapping_file.iter_exn mapping f in | ||
| Ao.flush_exn prefix |
There was a problem hiding this comment.
The result of this call isn't actually used (see _outcome above) so I'd say this can be left as-is too. Then no need for Ao.flush_exn.
There was a problem hiding this comment.
Removed the auxiliary flush_exn (if I read the code right, we still need to raise if error).
src/irmin-pack/unix/gc.ml
Outdated
| @@ -157,24 +159,26 @@ module Worker = struct | |||
|
|
|||
| (* Step 1. Open the files *) | |||
| [%log.debug "GC: opening files in RO mode"]; | |||
| let* fm = Fm.open_ro config in | |||
| let fm = Fm.open_ro config |> Errs.raise_if_error in | |||
| Errors.finalise (fun _outcome -> | |||
There was a problem hiding this comment.
Errors.finalise except both arguments to not raise errors.
Could you change the implementation of finalise to make it call the finiliser if f raised?
There was a problem hiding this comment.
very good point, thanks
icristescu
force-pushed
the
result_worker
branch
from
8d28645
to
b61da8b
Compare
icristescu
force-pushed
the
result_worker
branch
from
b61da8b
to
49e74a2
Compare
From #2039: the gc worker routine raises errors instead of propagating a result. The errors are catched at the end and converted it in a result for
write_gc_output.