-
Notifications
You must be signed in to change notification settings - Fork 4
Add KV cache for D1 index. Closes #306 #323
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
base: main
Are you sure you want to change the base?
Changes from 55 commits
fbbe19e
de444fb
0d3f60a
69291bf
a093922
e8608ef
8abd8ef
c504896
8f879bf
6b5f822
624b184
b66e758
5f18618
19cb854
ed6c067
4acf720
7101bb0
fe5d246
d149614
7158b9d
f4115c4
738399e
fd35178
93b0acc
d6b095f
117f878
982b25a
d4d1fec
d850cfa
9107a5d
56e807d
720fb95
41ecdb9
bc780ed
c158b88
1a72ebe
112e8dd
a818ad3
eb3c03f
174d6bd
97332cf
30cd11b
17de33a
3b2a846
2270be5
6e1e6cd
d786f26
0b891ef
8f694c5
7d84c55
e141fbe
14c9fbc
9aed565
248a038
48597ec
041f09f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,7 +64,7 @@ export async function handleFWSSDataSetCreated( | |
| /** | ||
| * Handle Filecoin Warm Storage Service service termination | ||
| * | ||
| * @param {{ DB: D1Database }} env | ||
| * @param {{ DB: D1Database; INDEX_CACHE_KV: KVNamespace }} env | ||
| * @param {any} payload | ||
| * @throws {Error} | ||
| */ | ||
|
|
@@ -78,4 +78,27 @@ export async function handleFWSSServiceTerminated(env, payload) { | |
| ) | ||
| .bind(String(payload.data_set_id)) | ||
| .run() | ||
| await clearDataSetIndexCache(env, payload.data_set_id) | ||
| } | ||
|
|
||
| /** | ||
| * @param {{ DB: D1Database; INDEX_CACHE_KV: KVNamespace }} env | ||
| * @param {string | number} dataSetId | ||
| */ | ||
| async function clearDataSetIndexCache(env, dataSetId) { | ||
| const { results } = await env.DB.prepare( | ||
| ` | ||
| SELECT data_sets.payer_address AS payerAddress, pieces.cid AS pieceCID | ||
| FROM data_sets | ||
| INNER JOIN pieces ON pieces.data_set_id = data_sets.id | ||
| WHERE data_sets.id = ? | ||
| `, | ||
| ) | ||
| .bind(String(dataSetId)) | ||
| .run() | ||
| await Promise.all( | ||
| results.map(async ({ payerAddress, pieceCID }) => { | ||
| await env.INDEX_CACHE_KV.delete(`${payerAddress}/${pieceCID}`) | ||
| }), | ||
|
Comment on lines
+100
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
I will evaluate both tomorrow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good, let's reevaluate |
||
| ) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.