Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gateway): more explicit IPFSBackend and no multi-range #369

Merged
merged 11 commits into from
Oct 2, 2023

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Jun 21, 2023

Also, switches to only handling single range requests rather than multiples.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #369 (dcd7a19) into main (76f8e3f) will decrease coverage by 0.69%.
The diff coverage is 34.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   66.44%   65.75%   -0.69%     
==========================================
  Files         204      208       +4     
  Lines       24621    25099     +478     
==========================================
+ Hits        16360    16505     +145     
- Misses       6838     7140     +302     
- Partials     1423     1454      +31     
Files Coverage Δ
gateway/metrics.go 79.23% <0.00%> (ø)
gateway/handler_unixfs_file.go 73.01% <86.66%> (+8.73%) ⬆️
gateway/handler.go 78.01% <28.57%> (-0.91%) ⬇️
gateway/handler_unixfs__redirects.go 54.34% <61.53%> (+0.58%) ⬆️
gateway/handler_block.go 69.76% <22.22%> (-14.02%) ⬇️
gateway/handler_codec.go 60.95% <65.00%> (-1.24%) ⬇️
gateway/handler_unixfs_dir.go 60.25% <44.82%> (-4.03%) ⬇️
gateway/gateway.go 81.88% <40.00%> (-11.87%) ⬇️
gateway/handler_defaults.go 31.76% <38.88%> (-5.89%) ⬇️
gateway/blocks_backend.go 42.05% <24.44%> (-1.69%) ⬇️
... and 1 more

... and 14 files with indirect coverage changes

@hacdias
Copy link
Member

hacdias commented Jun 22, 2023

@aschmahmann can we not support multiple ranges? Isn't that part of the specs? Also note that I'll be working on IPIP-412 soon, sow e might get some conflicts.

@aschmahmann aschmahmann force-pushed the feat/gateway-backend-more-explicit branch 3 times, most recently from db5809e to 1cffe61 Compare June 28, 2023 00:01
@aschmahmann aschmahmann force-pushed the feat/gateway-backend-more-explicit branch from 1cffe61 to 5bca4fc Compare July 6, 2023 15:50
@aschmahmann aschmahmann marked this pull request as ready for review July 11, 2023 16:47
@BigLep
Copy link
Contributor

BigLep commented Jul 11, 2023

2023-07-11 maintainer conversation: we shouldn't stress about multiple ranges. We have a metric for this against ipfs.io and haven't found any actual usage. This conformance test for multiple ranges can be moved to Kubo.
@aschmahmann will flip it out of draft when ready for review.

hacdias

This comment was marked as outdated.

@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from cd21e88 to a8d5f27 Compare July 26, 2023 09:11
@hacdias
Copy link
Member

hacdias commented Jul 26, 2023

This conformance test for multiple ranges can be moved to Kubo.

@BigLep @aschmahmann I don't understand how that helps. If we move the test to Kubo, it will still fail since we are removing the functionality on this PR and that functionality will simply no longer be available.

@aschmahmann
Copy link
Contributor Author

If we move the test to Kubo, it will still fail since we are removing the functionality on this PR and that functionality will simply no longer be available.

Correct. IIUC the right thing to do here is:

  1. Have the conformance tests test the HTTP guarantees around multiple ranges (e.g. that the server can send back the ranges, the first range, or none as long as it's clear from the response headers)
  2. If we end up having this functionality in just one implementation/consumer of boxo (e.g. kubo) then we test there

We can add back in the multiple range functionality, but IIUC from @lidel basically no one is using this so it's not worth the hassle at the moment.

@hacdias
Copy link
Member

hacdias commented Jul 26, 2023

Working on updating the tests here: ipfs/gateway-conformance#113

@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from b2a3b77 to 90fccc2 Compare July 26, 2023 13:12
@hacdias
Copy link
Member

hacdias commented Jul 26, 2023

@aschmahmann @lidel this PR is blocked until ipfs/gateway-conformance#113 is merged and released (ready for review), so that we can have the tests passing here and/or in Kubo's PR.

@hacdias hacdias changed the title feat(gateway): make backend api more explicit. feat(gateway): make backend api more explicit Jul 26, 2023
@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch 2 times, most recently from 1b58ebf to 6f95ad1 Compare July 31, 2023 11:19
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(submitting first-pass at the end of day)

gateway/blocks_backend.go Outdated Show resolved Hide resolved
gateway/blocks_backend.go Outdated Show resolved Hide resolved
gateway/blocks_backend.go Outdated Show resolved Hide resolved
gateway/blocks_backend.go Outdated Show resolved Hide resolved
gateway/errors.go Outdated Show resolved Hide resolved
gateway/handler_codec.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from 50fcc4e to 34b4055 Compare August 15, 2023 09:42
@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from 34b4055 to 35865d5 Compare August 18, 2023 06:37
@hacdias hacdias changed the title feat(gateway): make backend api more explicit feat(gateway): more explicit IPFSBackend, only support single-range requests Aug 18, 2023
@hacdias hacdias changed the title feat(gateway): more explicit IPFSBackend, only support single-range requests feat(gateway): more explicit IPFSBackend and no multi-range Aug 18, 2023
@hacdias hacdias requested a review from lidel August 18, 2023 08:09
@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from 35865d5 to 592fafc Compare August 22, 2023 12:28
@BigLep
Copy link
Contributor

BigLep commented Aug 22, 2023

2023-08-22 conversation: @lidel will review and figure out appropriate testing story (Boxo unit tests vs. conformance tests)

@BigLep
Copy link
Contributor

BigLep commented Aug 24, 2023

2023-08-24 conversation:
We are going to lean on conformance tests. @lidel is going to create a gateway-conformance issues for this.
It's a lesser evil to merge without the conformance tests than keep this open and let it diverge.
@aschmahmann will add some Boxo unit tests and flag the changed lines of potential concerns.
@aschmahmann will make sure we get the merge decision
(There is also some dance for keeping CI green with sharness.)

@BigLep
Copy link
Contributor

BigLep commented Sep 11, 2023

@aschmahmann : is this on track to land this week?

@BigLep
Copy link
Contributor

BigLep commented Sep 12, 2023

2023-09-12 conversation:

  • Do a gateway-conformance release as soon as possible: feat: range request helpers gateway-conformance#162 lands and passes.
  • To get green CI
  • Conformance release
  • Bubble up to Boxo
  • Bubble this PR into Kubo
  • Run this PR against Kubo version
  • Once Boxo CI passes
  • Switch to target Kubo master
  • Merge this PR
  • Bubble up Boxo commit to Kubo PR
  • Merge Kubo PR
  • Rerun Boxo CI

@hacdias hacdias force-pushed the feat/gateway-backend-more-explicit branch from 1d2b5c4 to dcd7a19 Compare October 2, 2023 14:11
@hacdias
Copy link
Member

hacdias commented Oct 2, 2023

CI is green in Kubo. As per what was agreed, I'm merging this once CI finishes here and bubbling it up.

@hacdias hacdias merged commit c28c847 into main Oct 2, 2023
13 of 14 checks passed
@hacdias hacdias deleted the feat/gateway-backend-more-explicit branch October 2, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants