RO syncs

[icristescu]

A straightforward implementation for RO sync. I'll add more tests (concurrent ones too) so for now its a draft.

The way I think ro_sync should be used is to catch exceptions for RO operations, and retry ro_sync in case of failure.

Depends on mirage/index#175

[samoht]

what happens in the case where the 3 stores are actually the same store? Do we have a way to make this more efficient?

[icristescu]

I left only one ro_sync call and added a comment.

[icristescu]

If during a merge, an RO instance calls ro_sync and then looks for a value, that was flushed to disk by the merge, it will lead to an exception raised instead of returning None (see https://github.com/mirage/irmin/pull/1008/files#diff-51f1262f591288eadf8719ee5f293b18R309).

I added some tests that use the force_merge function from index, so I switched to Private.Make in pack_index. I think this is fine, we are not allowed to use pack_index from outside irmin-pack, but maybe there is a cleaner way to do this?

[craigfe]

I added some tests that use the force_merge function from index, so I switched to Private.Make in pack_index. I think this is fine, we are not allowed to use pack_index from outside irmin-pack, but maybe there is a cleaner way to do this?

I'm very uneasy about that. The private API of Index is now being exposed by Irmin_pack.Index, and the tests get access to that API very easily (no references to Private or such). Indeed, constructing a Pack instance now requires the user to construct an Index with the private API.

If testing Index dependencies requires them having access to the private API, perhaps we need to rethink the public API 🙂

[icristescu]

I'm very uneasy about that. The private API of Index is now being exposed by Irmin_pack.Index, and the tests get access to that API very easily (no references to Private or such). Indeed, constructing a Pack instance now requires the user to construct an Index with the private API.

If testing Index dependencies requires them having access to the private API, perhaps we need to rethink the public API 🙂

I would like to have a Make_private functor for irmin pack that can use the private API of index. I tried and it leads to a lot of code duplication, so I gave up.

But we need to use the private API of index in this PR, the problematic behaviour of ro_sync only occurs during a merge.

[icristescu]

I added some tests that use the force_merge function from index, so I switched to Private.Make in pack_index.

I removed the private API of index, and used Index.filter and Index.flush instead.

[craigfe]

As discussed offline, the tricky case of "flush to index before indexed value has been flushed to pack" could be solved by having Index support a pre-flush callback. We would use that callback to flush to Pack.

[craigfe]

(And we should be sure to fsync that flush inside Pack, so as to keep the ordering correct even in the presence of crashes.)

[icristescu]

As discussed offline, the tricky case of "flush to index before indexed value has been flushed to pack" could be solved by having Index support a pre-flush callback. We would use that callback to flush to Pack.

I added the hook from mirage/index#189 in the latest commit, and the tests now work fine, no invalid_read is raised.

[craigfe]

Thanks. Will wait for Travis to wake up and then merge.