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

feat/cmds: hide peers info default in bitswap stat #5820

Merged

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Dec 5, 2018

Fixed: #5763

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

@overbool overbool requested a review from Kubuxu as a code owner December 5, 2018 06:36
@overbool overbool force-pushed the feat/cmds/hide-peers-in-bitswap-stat branch from eff877f to 52534b6 Compare December 5, 2018 06:36
@overbool overbool changed the title feat(cmds): hide peers info default in bitswap stat feat/cmds: hide peers info default in bitswap stat Dec 5, 2018
@Stebalien
Copy link
Member

In theory, I completely agree with this. Unfortunately, in practice, it changes a long-standing stable interface.

@whyrusleeping thoughts? At the moment, this command is kind of painful due to this issue.

@whyrusleeping
Copy link
Member

I pretty much always pipe this command to head -n10. It would not hurt my feelings if this got changed.

That said, 'bitswap partners' are really supposed to just be peers we are 'actively' trading data with. 'every peer you are connected to' really shouldnt be in that list..

@whyrusleeping
Copy link
Member

maybe for now, default verbose to true, that way we can use ipfs bitswap stat -v=false to get the trimmed output.

I'm also not against this change as is, but if we go forward with it, we should do a bit of due dilligence to see if anyone is parsing the output of this command and using it for something

@eingenito
Copy link
Contributor

I'd suggest not changing the default. It seems monitoring/troubleshooting related and therefore the kind of thing that people might well call in a script (like to check the health of their cluster or something). And because it's a little bit arcane it seems like marginal benefit for a change that's hard to estimate the impact of.

@Stebalien
Copy link
Member

And because it's a little bit arcane it seems like marginal benefit for a change that's hard to estimate the impact of.

So, as long as we don't change the json output, we should mitigate the damage (although yes, someone is probably parsing the text...).

Maybe a solution is to just fix this by only showing real bitswap peers. We can probably record a simple "active" refcount for each peer.

However, that will have to wait until the current bitswap refactor is done.

@eingenito
Copy link
Contributor

So, as long as we don't change the json output, we should mitigate the damage (although yes, someone is probably parsing the text...).

👍 Interesting.

Maybe a solution is to just fix this by only showing real bitswap peers. We can probably record a simple "active" refcount for each peer.

Flip-flop'ing here but if we're comfortable changing the default behavior in the future, and it has little (no?) worth now, maybe it's actually better to merge this and we'll improve the bitswap peers listing when we get to it. I guess right now this is roughly equivalent to ipfs swarm peers.

I can imagine lots of per-peer bitswap stats that might be neat but that would definitely change the json.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I've done some soul searching on this and, really, I don't think anyone will complain about this change. We're changing what is effectively a debugging interface (which not even the webui uses.

Everyone here seems quite happy because ipfs bitswap stat currently sucks so, (yolo).

License: MIT
Signed-off-by: Overbool <overbool.xu@gmail.com>
@Stebalien Stebalien force-pushed the feat/cmds/hide-peers-in-bitswap-stat branch from 52534b6 to ca4ba6c Compare March 5, 2019 03:12
@ghost ghost assigned Stebalien Mar 5, 2019
@ghost ghost added the status/in-progress In progress label Mar 5, 2019
@Stebalien Stebalien merged commit b32458d into ipfs:master Mar 5, 2019
@ghost ghost removed the status/in-progress In progress label Mar 5, 2019
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.

4 participants