Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

Support non-block requests #45

Closed
willscott opened this issue Feb 23, 2023 · 4 comments · Fixed by #51
Closed

Support non-block requests #45

willscott opened this issue Feb 23, 2023 · 4 comments · Fixed by #51

Comments

@willscott
Copy link
Contributor

the first top-level interface of caboose is as a blockstore.

we should work with @aschmahmann to understand what requests for car files rather than blocks might look like as they pass through this library.

My first inclination is something like

Fetch(urlPath string, cb func(io.ReadCloser) error) error

where cb would validate if a response matched expected response (and would be called 0+ times, and at most 1 time where it returned a nil error)

@aschmahmann
Copy link

aschmahmann commented Feb 24, 2023

IMO we should try and keep Caboose as limited to Saturn concerns as we can afford to. This means trying to keep it out of the IPLD game as much as we can.

My understanding of what we need out of this interface boundary is:

  1. Caboose needs to understand if the data received from the L1 is garbage/insufficient so it can report that to the orchestrator and/or take it into account in future queries
  2. Caboose doesn't want to care where the blocks from the CAR end up going
  3. The Caboose caller (e.g. bifrost-gateway) doesn't want to keep redownloading the same data they already have if they don't want to.
    • @lidel may have opinions here on how this plays out, but while the easiest to start with is redownloading everything during a failure an alternative is basically to resume/re-execute he query based on the data we've already received (e.g. for a large file send a range-request for where we left off)

So we can probably go with your initial proposal Fetch(urlPath string, cb func(io.ReadCloser) error) error if there are sentinel errors returned from the callback to help Caboose figure out what to do/what went wrong, An alternative that's a bit more explicit might be:
Fetch(urlPath string) (data io.ReadCloser, saturnReportBack func(some enum or struct), err error) where the report-back could be a channel, callback, or whatever and the thing that goes into the callback is whatever information Saturn wants/needs.

My thinking is that instead of the caller trying to communicate with Caboose about whether to do a retry via error sentinels they can just call Fetch again with the same or different URL if they need more data and the error wasn't obviously something Caboose can handle (e.g. a 500)

Thoughts?

@willscott
Copy link
Contributor Author

@aschmahmann both can make sense - the semantics of

Fetch(urlPath string, cb func(io.ReadCloser) error) error

make more sense in my head.

We expect there to be some level of failure cases where cb would end up triggering multiple times. In the reportBack alternative, the caller has to re-call Fetch on a failure. That would mean caboose would have to implicitly keep state between the subsequent calls in a way that feels like it would get messy.
(e.g. we'd want Fetch to hold some sort of caboose-internal state struct so that it can know the retry is a second attempt and to try the fallback L1 for that request.) Having that end up in the interface feels ugly to me.

I can prototype Fetch(urlPath string, cb func(io.ReadCloser) error) error and we can see if it's painful to deal with?

@aschmahmann
Copy link

Yeah, I'd hope the implementation internally isn't so dependent on this that we can change it later if need be. While you're prototyping try and keep in mind what you want the caller contract to be when the errors are streaming (i.e. interrupted CARs) rather than 4xx/5xx errors.

@willscott
Copy link
Contributor Author

#51 exposes the basic interface.

I went with the interface

type ErrPartialResponse struct {
	error
	StillNeed []string
}

As the way to handle partial responses - the cb would return this error with the sub-paths of data remaining as the way to efficiently handle resumptions.

One thing this likely means is that i'm expect to update as i get through tests and support for efficient handling of those partial continuations is that the callback will evolve to
cb func(path string, reader io.ReadCloser) error
so that the subsequent invocation knows the subset the returned reader is fulfilling.

This should be enough for initial integration in starting to craft what the car validation / fullifilling callback looks like.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants