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

[WIP] command: bitswap ledger #2814

Closed
wants to merge 5 commits into from
Closed

Conversation

pfista
Copy link
Contributor

@pfista pfista commented Jun 6, 2016

Starting work for #437

@pfista
Copy link
Contributor Author

pfista commented Jun 6, 2016

@whyrusleeping Would like your input if you think this is the right direction. I don't know if exporting engine and ledger is the best approach.

@whyrusleeping
Copy link
Member

@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.

@pfista pfista changed the title [WIP] command: bitswap ledger command: bitswap ledger Jun 6, 2016
@pfista pfista changed the title command: bitswap ledger [WIP] command: bitswap ledger Jun 6, 2016
@whyrusleeping
Copy link
Member

This looks a lot cleaner. I'll need to take a better look in the morning, but LGTM right now 👍

@pfista
Copy link
Contributor Author

pfista commented Jun 7, 2016

It's worth mentioning too if this is not really needed anymore we should
just close the issue to avoid adding anything unnecessary.
On Tue, Jun 7, 2016 at 01:37 Jeromy Johnson notifications@github.com
wrote:

This looks a lot cleaner. I'll need to take a better look in the morning,
but LGTM right now 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2814 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAsdznjhukLM6KSHu8C-P-2O9yjBPE9Fks5qJS3cgaJpZM4IvJKZ
.

@whyrusleeping
Copy link
Member

@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)

@RichardLitt
Copy link
Member

Subscribing.


func (e *Engine) LedgerSnapshot(p peer.ID) *LedgerSnapshot {
snapshot := &LedgerSnapshot{
DebtRatio: e.findOrCreate(p).Accounting.Value(),
Copy link
Member

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()
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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>
License: MIT
Signed-off-by: Michael Pfister <pfista@gmail.com>
License: MIT
Signed-off-by: Michael Pfister <pfista@gmail.com>
@whyrusleeping
Copy link
Member

closing in favor of #2852

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.

3 participants