[go: up one dir, main page]

[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_fetch implementation 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.

  1. First approach is to change With_remote_fetch functor's fetch function to also take api_version. This was already tried in the following commit 5bb2896a.
    • One could argue that since Remote module so far has only been used to call API's of different dac nodes, the fetch function could be api_version dependent. To be more explicit we could rename With_remote_fetch to With_remote_api_fetch?
    • The argument that we want to later on leverage With_remote_fetch for 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 where Remote is used requires calling other nodes APIs.
  2. Alternative approach is to keep With_remote_fetch as is. Define new API_VERSION module and define new Make_remote functor 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 betweenval remote_store: Rpc_services.Api.version -> (Page_store.S with type configuration = remote_configuration) vs Remote_V0 and Remote_V1
      • The cost of resolving modules at runtime.
        • Provided this is even possible of course (see the points above)
  3. Change our abstractions entirely. Saying that Page_store.FileSystem and Page_store.Remote is the same abstraction, namely described with Page_store.S module 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.