-
-
Notifications
You must be signed in to change notification settings - Fork 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
[WIP] command: bitswap ledger #2814
Conversation
@whyrusleeping Would like your input if you think this is the right direction. I don't know if exporting |
@pfista the problem with working on this command now is that the ledger information isnt really used. Although i guess it might be interesting just so we can see how much data we've sent to each peer. I think instead of directly exporting the ledger, we should add a way to get a snapshot of a ledger. That way we avoid any risk of race conditions and keep things more well contained. |
This looks a lot cleaner. I'll need to take a better look in the morning, but LGTM right now 👍 |
It's worth mentioning too if this is not really needed anymore we should
|
@pfista i like this command. I just need to get to reviewing it (sorry about the delay, the decentralized web conference and nodeconf took a fair bit of time away from github) |
Subscribing. |
|
||
func (e *Engine) LedgerSnapshot(p peer.ID) *LedgerSnapshot { | ||
snapshot := &LedgerSnapshot{ | ||
DebtRatio: e.findOrCreate(p).Accounting.Value(), |
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 think we're going to have to worry about locking a bit here. Lets make a single call to findOrCreate
which is threadsafe on its own, then call lock on the returned ledger, defer the unlock, and then take the information we need from it
|
||
func (e *Engine) LedgerSnapshot(p peer.ID) *LedgerSnapshot { | ||
e.lock.Lock() | ||
defer e.lock.Unlock() |
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.
@whyrusleeping Looking at the other code I noticed you're locking on the engine. Let me know if instead I should actually lock on the ledger like you mentioned in your previous comment.
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 will deadlock. findOrCreate
takes the same lock that youre taking here. The correct way to do it will be to take the lock on the ledger you get back.
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.
Oh duh. I hadn't rebased yet, the old findOrCreate wasn't locking on engine...
License: MIT Signed-off-by: Michael Pfister <pfista@gmail.com>
License: MIT Signed-off-by: Michael Pfister <pfista@gmail.com>
License: MIT Signed-off-by: Michael Pfister <pfista@gmail.com>
2ff35ab
to
db1f6f0
Compare
closing in favor of #2852 |
Starting work for #437