This repository has been archived by the owner on Apr 18, 2024. It is now read-only.
feat: switch to boxo and fix CAR fetch timeouts #68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: ipfs/boxo#215 We use commit from ipfs/boxo#176 (comment) to unblock work on ipfs-inactive/bifrost-gateway#61
lidel
added a commit
to ipfs-inactive/bifrost-gateway
that referenced
this pull request
Mar 29, 2023
Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61
lidel
added a commit
to ipfs-inactive/bifrost-gateway
that referenced
this pull request
Mar 29, 2023
Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61
lidel
changed the title
19s is not enough for fetching CAR stream of unknown length, every bigger request was failing. If we need to pick some ceiling, 30m sound like a good starting point (this is when CAR stream got timeouted on the old ipfs.io).
lidel
force-pushed
the
refactor/migrate-to-boxo
branch
from
206d01a
to
5b37545
Compare
lidel
commented
Mar 29, 2023
Comment on lines
+153
to
+162
| // TODO: Ideally, we would have additional "PerRequestInactivityTimeout" | ||
| // which is the amount of time without any NEW data from the server, but | ||
| // that can be added later. We need both because a slow trickle of data | ||
| // could take a large amount of time. | ||
| requestTimeout := DefaultSaturnCarRequestTimeout | ||
| if isBlockRequest { | ||
| requestTimeout = DefaultSaturnBlockRequestTimeout | ||
| } | ||
|
|
||
| reqCtx, cancel := context.WithTimeout(ctx, requestTimeout) |
There was a problem hiding this comment.
💭
Implementing "PerRequestInactivityTimeout" would help a lot – we could then have this 30m timeout as a hard ceiling (or even raise it), but then have the same timeout for block and for CAR when L1 did not send any new bytes for some time.
lidel
commented
Mar 29, 2023
Comment on lines
+83
to
+84
| const DefaultSaturnBlockRequestTimeout = 19 * time.Second | ||
| const DefaultSaturnCarRequestTimeout = 30 * time.Minute |
There was a problem hiding this comment.
ℹ️ Had to add separate timeout for CARs.
Not feeling strongly about 30m, but that is for how long I was able to stream wikipedia DAG from the old gateway, so a good starting point.
Undef cid was displayed as "b" (multibase prefix)
2 tasks
lidel
requested review from
willscott and
aarshkshah1992
and removed request for
willscott
lidel
commented
Mar 29, 2023
| @@ -122,7 +130,16 @@ type ErrCoolDown struct { | |||
| } | |||
|
|
|||
| func (e *ErrCoolDown) Error() string { | |||
| return fmt.Sprintf("multiple saturn retrieval failures seen for CID %s/Path %s, please retry after %s", e.Cid, e.Path, humanRetry(e.retryAfter)) | |||
There was a problem hiding this comment.
ℹ️ This was producing very confusing log errors failures seen for CID b/Path due to e.Cid being undefined, and printed not sanitized input from the user.
aarshkshah1992
approved these changes
Mar 30, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR switches to
boxo(why: ipfs/boxo#215).It is possible we will be also adding things related to ipfs/boxo#176 and ipfs-inactive/bifrost-gateway#61 to this PR, so everything can be reviewed and merged together.
(Usually trying to have smaller PRs, but we have too many PRs caused by
boxorename, trying to minimize overhead related to this unplanned rename)TODO
go-libipfstoboxo(Context: Rename go-libipfs to boxo ipfs/boxo#215)TBD, could be follow-up PR