-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: sector index yugabyte implementation #11135
Conversation
1abbebe
to
652ee04
Compare
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.
First pass, looks quite straightforward, the only thing is really addressing the todos, especially the transactional StorageAttach (that has to be correctly transactional, often related paths will be attached by multiple workers at the same time, e.g. when workers are bulk-restarted)
return out, nil | ||
} | ||
|
||
func union(a, b []string) []string { |
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.
lo.Union() ?
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.
First pass, this looks like a real thing now, just a few comments
storage/paths/db_index.go
Outdated
_, err := dbi.lock(ctx, sector, read, write) | ||
if err != nil { | ||
return err | ||
} |
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.
This probably should retry / backoff / block when we don't get the lock immediately
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.
Added. Could you review the parameters for the retries and wait time and see if they make sense? @magik6k
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.
This is getting really close,
Few comments on the locking side of things, and it would be nice to avoid passing a query built through Sprintf in StorageBestAlloc. Other than that this looks shippable
(also CI seems sad, but not sure if this is related to this PR or feat/sturdypost) |
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.
Mostly small nitpicky stuff, everything looks pretty solid now, next review should be shipping the code
Related Issues
#11025
Proposed Changes
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps