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

Remove provider query manager #536

Open
wants to merge 1 commit into
base: remove-content-routing-from-bitswap-network
Choose a base branch
from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jan 3, 2024

Based on #535

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (remove-content-routing-from-bitswap-network@2dbd42a). Click here to learn what that means.

❗ Current head f3d731b differs from pull request most recent head 2e813d8. Consider uploading reports for the commit 2e813d8 to get more accurate results

Impacted file tree graph

@@                              Coverage Diff                               @@
##             remove-content-routing-from-bitswap-network     #536   +/-   ##
==============================================================================
  Coverage                                               ?   65.31%           
==============================================================================
  Files                                                  ?      206           
  Lines                                                  ?    25244           
  Branches                                               ?        0           
==============================================================================
  Hits                                                   ?    16488           
  Misses                                                 ?     7281           
  Partials                                               ?     1475           
Files Coverage Δ
bitswap/bitswap.go 69.51% <ø> (ø)
bitswap/client/client.go 86.86% <100.00%> (ø)
...tswap/client/internal/messagequeue/messagequeue.go 84.92% <ø> (ø)
bitswap/network/ipfs_impl.go 78.86% <100.00%> (ø)
bitswap/options.go 44.44% <100.00%> (ø)
bitswap/server/server.go 59.75% <100.00%> (ø)
bitswap/testnet/peernet.go 38.46% <100.00%> (ø)
blockservice/test/mock.go 100.00% <100.00%> (ø)
examples/unixfs-file-cid/main.go 41.21% <100.00%> (ø)
gateway/blocks_backend.go 42.23% <ø> (ø)
... and 4 more

@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from efaeefb to 3a493b8 Compare January 5, 2024 15:56
Jorropo added a commit to ipfs/kubo that referenced this pull request Jan 5, 2024
@hacdias hacdias changed the base branch from main to remove-content-routing-from-bitswap-network January 8, 2024 12:59
@Jorropo Jorropo changed the base branch from remove-content-routing-from-bitswap-network to main January 11, 2024 17:20
@Jorropo Jorropo changed the base branch from main to remove-content-routing-from-bitswap-network January 11, 2024 18:07
@Jorropo
Copy link
Contributor Author

Jorropo commented Jan 11, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Comment on lines +416 to +418
if span.IsRecording() {
span.SetAttributes(attribute.Stringer("cid", c))
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be done directly? AFAIK, it won't call .String unless it's indeed recording. I think it could indeed be more problematic if directly passed as a string and needed to encode it every time. But here it seems fine. Is there another reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It literally always call .String instantly:
https://github.com/open-telemetry/opentelemetry-go/blob/98b32a6c3a87fbee5d34c063b9096f416b250897/attribute/kv.go#L85

// Stringer creates a new key-value pair with a passed name and a string
// value generated by the passed Stringer interface.
func Stringer(k string, v fmt.Stringer) KeyValue {
	return Key(k).String(v.String())
}

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

as on the previous stack, it looks like it does what it says, just a comment inline

@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from bed8453 to 15661e4 Compare January 16, 2024 09:19
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from d414841 to 5eb2dbc Compare February 14, 2024 18:40
@Jorropo Jorropo requested a review from lidel as a code owner February 14, 2024 18:40
@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from 15661e4 to f3d731b Compare February 14, 2024 18:40
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 5eb2dbc to 0cd80af Compare February 16, 2024 13:47
@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from f3d731b to 1c8b6c9 Compare February 16, 2024 13:47
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 0cd80af to 6740401 Compare February 16, 2024 13:48
@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from 1c8b6c9 to 5104983 Compare February 16, 2024 13:48
Closes: #172
See #172 (comment) too.

providerQueryManager took care of:
- Deduping multiple sessions doing find providers for the same CID.
- limiting global find providers.

None of which we care:
- This is rare, if this happens it's fine to run the same query twice.
  If we care then we should make a deduping content router so we can inject it anywhere a content router is needed.
- It's fine to allow one concurrent find peer per session. No need to limit this at 6 globally after that, it's a great way to stall nodes doing many queries.
@Jorropo Jorropo force-pushed the remove-content-routing-from-bitswap-network branch from 6740401 to 2dbd42a Compare February 16, 2024 14:09
@Jorropo Jorropo force-pushed the remove-provider-query-manager branch from 5104983 to 2e813d8 Compare February 16, 2024 14:09
Jorropo added a commit to ipfs/kubo that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants