[DAC-question] Fetch function should be API agnostic
Discussion started on this thread !8554 (comment 1367637311).
The idea is that Page_store.With_remote_fetch functor (see) is general and does not require the notion of api_version. This is mainly due to:
The fetch function should be generic enough to not depend on the concept of API version. If later on one wants to leverage
With_remote_fetchimplementation that does not depend on the API at all (e.g. it might want to read pages from a remote document store), then it should be able to do so.
Here is one concern though. The only existing use case in our codebase of using With_remote_fetch is for defining Page_store.Remote module (see). As from the docstring:
(** A [Page_store] implementation backed by the local filesystem, which
uses a connection to a Dac node to retrieve pages that are not
saved locally. *)
module Remote : S with type configuration = remote_configuration
we can see that Remote module is actually used to "hide" potential explicit API calls to Coordinator (eventually Committee members) during the de-serialization process. This happens e.g., when subscriber receives new root hash from coordinator or when we handle get missing page request.
As such it is clear that Remote module is API dependent. The question is now how we propagate this dependency.
- First approach is to change
With_remote_fetchfunctor'sfetchfunction to also takeapi_version. This was already tried in the following commit 5bb2896a.- One could argue that since
Remotemodule so far has only been used to call API's of different dac nodes, thefetchfunction could beapi_versiondependent. To be more explicit we could renameWith_remote_fetchtoWith_remote_api_fetch? - The argument that we want to later on leverage
With_remote_fetchfor the case where we don't depend on the API (e.g. remote document system) is not necessarily correct, as we have seen that the places whereRemoteis used requires calling other nodes APIs.
- One could argue that since
- Alternative approach is to keep
With_remote_fetchas is. Define newAPI_VERSIONmodule and define newMake_remotefunctor as suggested here. Attempt to do so was done in the following commit a3e6d998. The main problems were:-
Module type is not a module: see first and mainly second comment. - A potential remedy to this might be to "change the pages encoding to take the save function from the remote page store in input, rather than the whole remote page store module" (suggested here)
- This was not explored yet.
- Additionally there has been a consideration between
val remote_store: Rpc_services.Api.version -> (Page_store.S with type configuration = remote_configuration)vsRemote_V0andRemote_V1- The cost of resolving modules at runtime.
- Provided this is even possible of course (see the points above)
- The cost of resolving modules at runtime.
-
- Change our abstractions entirely. Saying that
Page_store.FileSystemandPage_store.Remoteis the same abstraction, namely described withPage_store.Smodule type might be a bit misleading. As the former is abstraction for the current node native storage and the later is abstraction for calling other's actors API and not abstraction for remote (or backup) storage native to the current node.