-
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: piecereader: Allow parallel access #10913
Conversation
storage/sealer/piece_reader.go
Outdated
} | ||
|
||
func (p *pieceReader) init() (_ *pieceReader, err error) { | ||
stats.Record(p.ctx, metrics.DagStorePRInitCount.M(1)) | ||
|
||
p.seqMCtx, _ = tag.New(p.ctx, tag.Upsert(metrics.PRReadType, "seq")) |
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.
context.Context is never supposed to be stored in a struct, so:
can we have init(ctx context.Context) ?
Here and the next line, can tag.New take that Context and return it as well (since it should be non-destructive).
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.
Yeahh, moved the ctx into init params, but the MCtx
ones will probably have to stay in that struct - opencensus sadly needs a context to record stats on (technically we could hack into it's internal packages, but that's even more gnarly)
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.
The idea of passing context around is that it follows the goroutine & opencensus is producing a stat on that. Is this stat on a shared thing that cannot be expressed individually by the callers?
415089f
to
b58daf5
Compare
Looks good. Let me know when you're ready for me to approve.
…On Thu, May 25, 2023 at 9:08 AM Łukasz Magiera ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In storage/sealer/piece_reader.go
<#10913 (comment)>
:
> }
func (p *pieceReader) init() (_ *pieceReader, err error) {
stats.Record(p.ctx, metrics.DagStorePRInitCount.M(1))
+ p.seqMCtx, _ = tag.New(p.ctx, tag.Upsert(metrics.PRReadType, "seq"))
Yeahh, moved the ctx into init params, but the MCtx ones will probably
have to stay in that struct - opencensus sadly needs a context to record
stats on (technically we could hack into it's internal packages, but that's
even more gnarly)
—
Reply to this email directly, view it on GitHub
<#10913 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOU4LTM5OAWVQP575CMLXDXH5RURANCNFSM6AAAAAAYLYA77E>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Merged master into here so that the previous commits can still be depended on in boost Should be mergeable now. |
Related Issues
Closes #10532
Replaces #10895
Proposed Changes
Additional Info
This PR is based on v1.23.0 so that it's possible to test with boost
Proposed Changes
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