-
Notifications
You must be signed in to change notification settings - Fork 96
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
bitswap/client: add option to disable duplicated block stats #195
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Codecov Report
@@ Coverage Diff @@
## main #195 +/- ##
=======================================
Coverage 65.65% 65.66%
=======================================
Files 207 207
Lines 25135 25141 +6
=======================================
+ Hits 16503 16508 +5
- Misses 7165 7169 +4
+ Partials 1467 1464 -3
|
I don't think anyone is using this to compare against the actual blockstore, I think it's more meant when you download the same block from multiple nodes. |
I am not sure what this stat is being used for or why it is implemented like this, I just want the ability to disable it. |
@Jorropo any objection to merging this? |
Ping @Jorropo |
1518fcb
to
9b8b3d7
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.
That fine, it solves the problem and allows to optin.
@hsanjuan sorry for the delay, this code is good and doesn't make anything worst even if it could be optimized. |
Keeping a counter should not require query-ing the blockstore repeteadly for every single block received, so I have added an option to disable it. Currently there is the assumption that doing Has() calls is cheap. That can be correct for most cases but also badly incorrect for others.
... when duplicated block stats are disabled.
9b8b3d7
to
4e7a9a3
Compare
@Jorropo now without allocating an empty slice |
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.
LGTM
A test would be good to catch regressions but I think this is fine. |
Merge and will do a follow up with a test some day. 👀 |
Keeping a counter should not require query-ing the blockstore repeteadly for every single block received, so I have added an option to disable it.
Currently there is the assumption that doing Has() calls is cheap. That can be correct for most cases but also badly incorrect for others.