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

Parallelize locator lookups #36

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Parallelize locator lookups #36

wants to merge 2 commits into from

Conversation

hannahhoward
Copy link
Member

Goals

Enable usage of the Indexer's multihash lookup

Implementation

  • Extract common logic in batching to its own class
  • Modify #executeReadClaims to call with multiple hashes
  • Modify #internalReadClaims to put requests in a queue if unless explicitly told to be synchronous

For discussion

I'd like some feedback before I finish testing and fix some of the tracing. anecdotally, this seems a win in testing with the indexer.

Copy link
Member

@fforbeck fforbeck left a comment

Choose a reason for hiding this comment

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

It looks good to me. Added one minor suggestion. Two questions, not necessarily affected by this change, but still related 1) is it ok to clone the blob before returning it in the resolveRequests? 2) if the fetch blob fail in processBatch, then it stops the flow, should we log the error before the iteration break?

*/

const MAX_BATCH_SIZE = 16
const MAX_BATCH_SIZE = 15
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Expose as a configurable parameter if batch sizes need to vary based on CF worker limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants