-
Notifications
You must be signed in to change notification settings - Fork 0
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: fetch payload CIDs from piece indexer #31
Conversation
Signed-off-by: Miroslav Bajtoš <[email protected]>
backend/bin/deal-observer-backend.js
Outdated
const LOOP_NAME = 'Piece Indexer' | ||
while (true) { | ||
const start = Date.now() | ||
const queryLimit = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we come up with this magic number? It defines how many active deals we look at at a time. It looks like by tuning it we control the trade off between processing speed (how many deals to update in a loop iteration) and memory usage (but not concurrency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the details; it's best to ask @NikolasHaimerl. Your description looks correct to me.
This will be easy to tweak later, I think we should pick some reasonable number for now and move on. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quickly skimmed throught the latest version and it looks very reasonable to me. I don't see any major issues/blockers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have piece-indexer - see https://github.com/CheckerNetwork/piece-indexer
How about calling this file piece-indexer-client.js
?
(Not blocking this PR from landing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I found that confusing as well. Will fix
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This MR proposes the following changes:
The lookup operation for piece CIDs is not yet complete. The piece cid indexer does not have all combinations of minerID and piece CID for any built-in actor event. A follow-up PR which deals with this issue is described in this issue.
We do not want to add the piece indexer loop to the list of executables for now. This should be done as soon as missing payloads are also dealt with.