feat: report remaining egress quotas #583
Conversation
akronim26
requested review from
bajtos,
juliangruber and
pyropy
as code owners
akronim26
changed the title
bajtos
changed the title
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
Thank you for the contribution; this is a good start!
Can you please revert the changes made in all worker-configuration.d.ts files? They are not relevant to this pull request and make the diff difficult to review. (Feel free to open a different pull request to update those .d.ts files.)
Please add some tests to verify your implementation.
We need to discuss how to handle the following use case:
- The client requests
(walletAddress,pieceCid)that's in our system, but where no dataset has sufficient egress quota. - FilBeam should respond with
429 Too Many Requests, and response headers should indicate the remaining CDN & cache-miss egress quotas.
If we think that's out of the scope of this pull request, then that's fine, and let's open a follow-up pull request.
| const contentLengthHeader = | ||
| retrievalResult.response.headers.get('content-length') | ||
| const estimatedEgress = contentLengthHeader | ||
| ? Number.parseInt(contentLengthHeader, 10) || 0 | ||
| : 0 | ||
| const remainingCdn = | ||
| retrievalCandidate.cdnEgressQuota - BigInt(estimatedEgress) | ||
| const remainingCacheMiss = | ||
| retrievalCandidate.cacheMissEgressQuota - BigInt(estimatedEgress) |
There was a problem hiding this comment.
I have mixed feelings about relying on content-length. It's not guaranteed that the response will have this header. I am concerned that the remaining egress reported by FilBeam will be unpredictable.
Also, there may be other requests in progress that will further reduce the remaining egress quotas - I think it's impossible to give the client a precise value.
I propose simplifying the report and reporting the remaining egress quota as it was at the start of the response, at the time sent back the headers.
@pyropy @juliangruber thoughts?
There was a problem hiding this comment.
I have mixed feelings about relying on content-length. It's not guaranteed that the response will have this header. I am concerned that the remaining egress reported by FilBeam will be unpredictable.
@bajtos You once mentioned that content length is encoded in CID V2, would it be possible to use that instead of the content-length headers?
Also, there may be other requests in progress that will further reduce the remaining egress quotas - I think it's impossible to give the client a precise value.
That's true, we should document that this may report an imprecise amount back to the user.
I propose simplifying the report and reporting the remaining egress quota as it was at the start of the response, at the time sent back the headers.
That might be a good start. I think these headers might be used to raise alerts and trigger top-ups (either manual or automated) and not for precise accounting so that could work.
There was a problem hiding this comment.
I have mixed feelings about relying on content-length. It's not guaranteed that the response will have this header. I am concerned that the remaining egress reported by FilBeam will be unpredictable.
@bajtos You once mentioned that content length is encoded in CID V2, would it be possible to use that instead of the
content-lengthheaders?
Yes, we can extract the piece size from the CID, but this will not work for range requests (e.g. when downloading a 1MB chunk of a video file, as browsers typically do).
There was a problem hiding this comment.
@bajtos +1 to simplifying the report, first get it simple and correct, we can improve it later
This reverts commit cf5de19b6020f03b307e652a7a29bd074a160316.
I am following the thread regarding content-length vs. the CID-based approach. I haven't finalized this part of the code or the tests yet because I wanted to make sure that I am aligned with the preferred approach. imo, the proposal to report the quota as it was at the start of the response seems like a good approach, especially given the range request issues @bajtos mentioned. Should I go ahead and implement that simplified version, or would you prefer I wait until you have reached a final decision? |
|
@akronim26 please go ahead with the simplified version 👍 |
pyropy
left a comment
pyropy
left a comment
There was a problem hiding this comment.
Let's remove remaining egress validation as that duplicates logic in getRetrievalCandidatesAndValidatePayer.
| ) | ||
| response.headers.set( | ||
| 'FB-Cdn-Egress-Remaining', | ||
| String(retrievalCandidate.cdnEgressQuota), |
There was a problem hiding this comment.
Are we confident it's good enough to respond with the quota for this one retrieval candidate? This means if you have stored with multiple SPs, you will get different responses randomly.
What about instead summing up the quotas of all retrieval candidates, since that's effectively how much you have left?
There was a problem hiding this comment.
Okay, got it. There can be multiple retrieval candidates, so we should sum up the quotas from all of them. I'll make the change.
There was a problem hiding this comment.
I propose returning two sets of headers: one with the remaining quotas for the selected data-set (as indicated in the X-Data-Set-ID header), and another for the total remaining quota.
If we feel the per-data-set quotas are not needed, then I am okay to not implement such response headers.
Either way, let's rename the headers returning the sum of all quotas to clearly indicate that the value is a sum across all datasets.
juliangruber
changed the title
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
You are making great progress! 👏🏻
4 participants
Fixes #378