Add KV cache for D1 index. Closes #306 #323
Conversation
| findInBadBits(env, pieceCid), | ||
| ]) | ||
| const indexCacheKey = `${payerWalletAddress}/${pieceCid}` | ||
| let [dataSetId, serviceUrl] = |
There was a problem hiding this comment.
Is it worth exploring the possibility of caching multiple datasets that share the same indexCacheKey? Users may have the same piece stored across multiple CDN-enabled data sets. If we only cache information for one data set, users could face retrieval failures when the cached data set's egress limit is reached, even though the same piece exists in other datasets.
Apart from that it would've also be nice to cache other info like egress usage and remaining egress quota (maybe not in this pull-request).
There was a problem hiding this comment.
This is a great point. I missed it for so long! I will think about it. Immediate thoughts:
- store an array of possible pieces as the value
- store multiple kv pairs, and perform a list() (slower)
- rotate the cache value after retrieval (pick a different possible piece)
Co-authored-by: Miroslav Bajtoš <[email protected]>
Co-authored-by: Srdjan <[email protected]>
Co-authored-by: Srdjan <[email protected]>
Co-authored-by: Srdjan <[email protected]>
Co-authored-by: Miroslav Bajtoš <[email protected]>
…am/worker into update/move-bad-bits-to-kv
| results.map(async ({ payerAddress, pieceCID }) => { | ||
| await env.INDEX_CACHE_KV.delete(`${payerAddress}/${pieceCID}`) | ||
| }), |
There was a problem hiding this comment.
Can this run into the limit of KV calls we can make per worker invocation? (I vaguely remember the number 1000.)
I think it's not likely for a long time, so we don't need to worry about that too much right now.
But it would be nice to have some visibility, so that we know early when we have a user approaching 1000 pieces stored. For example, we can have a Grafana chart with an alert where we show the value returned by a SQL query like the following one:
SELECT MAX(COUNT(*))
FROM pieces INNER JOIN data_sets ON pieces.data_set_id = data_sets.id
GROUP BY payer_addressI propose to open a follow-up tech-debt issue.
The question is whether we need this for the GA launch, and I don't think so.
Thoughts?
There was a problem hiding this comment.
Oh right it can happen, when there are at least 1000 pieces in a data set for example. I don't see this case as unlikely.
I see two options going forward:
- use queues
- use the REST API, which has higher batch limits
I will evaluate both tomorrow
There was a problem hiding this comment.
Yes, on second thought, I also concluded that the limit of 1000 pieces per dataset is too low, and we need to explore other options.
Considering the complexities, maybe we should put this performance optimisation on hold until the GA launch. WDYT?
There was a problem hiding this comment.
sounds good, let's reevaluate
|
@juliangruber please get @pyropy's approval before landing this change. His comment about a potential design issue seems relevant to me. |
| ]) | ||
|
|
||
| const indexCacheKey = `${payerWalletAddress}/${pieceCid}` | ||
| const [indexCacheValue, isBadBit] = await Promise.all([ |
There was a problem hiding this comment.
We're also going to need to store egress quota inside the KV store as we're not going to query database unless indexCacheValue is null or undefined.
How are we supposed to update these values given that KV store update is not a atomic operation?
|
Converting back to draft, as we're deprioritizing this in favor of ipfs/egress/x402 work |
As in #315, TS errors that I don't know how to fix are making CI fail.Closes #306