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.

Sign up for GitHub

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

Jump to bottom

Move providing responsabilities from bitswap to blockservice #677

Closed
wants to merge 2 commits into from
Closed

Move providing responsabilities from bitswap to blockservice #677

wants to merge 2 commits into from

Conversation

gammazero
Copy link
Contributor

  • bitswap/server: remove provide
  • blockservice: add session workaround to work with wrapped blockservices
  • blockservice: add WithProvider option
  • blockservice: remove session embeding in context

Replaces #534 by @Jorropo which was outdated enough to make merging difficult.

@gammazero
Move providing responsabilities from bitswap to blockservice
29faea9 
- bitswap/server: remove provide
- blockservice: add session workaround to work with wrapped blockservices
- blockservice: add WithProvider option
- blockservice: remove session embeding in context

Replaces #534 by @Jorropo which was outdated enough to make merging difficult.
@gammazero gammazero requested review from lidel and a team as code owners September 30, 2024 23:20
@gammazero gammazero mentioned this pull request Sep 30, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Sep 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 49.18033% with 31 lines in your changes missing coverage. Please review.

Project coverage is 60.17%. Comparing base (19a402b) to head (6af1b50).
Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
blockservice/blockservice.go 54.76% 13 Missing and 6 partials ⚠️
blockservice/providing_blockstore.go 14.28% 11 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
- Coverage   60.36%   60.17%   -0.20%     
==========================================
  Files         243      244       +1     
  Lines       30953    30867      -86     
==========================================
- Hits        18684    18573     -111     
- Misses      10612    10626      +14     
- Partials     1657     1668      +11
Files with missing lines Coverage Δ
bitswap/bitswap.go 69.51% <ø> (+2.07%) ⬆️
bitswap/options.go 33.33% <ø> (-3.04%) ⬇️
bitswap/server/server.go 55.73% <100.00%> (-8.19%) ⬇️
gateway/backend_blocks.go 45.64% <ø> (+0.15%) ⬆️
blockservice/providing_blockstore.go 14.28% <14.28%> (ø)
blockservice/blockservice.go 74.41% <54.76%> (-3.82%) ⬇️

... and 10 files with indirect coverage changes

@gammazero gammazero marked this pull request as draft October 22, 2024 16:44
@gammazero
Copy link
Contributor Author

Needs kubo PR to test

@aschmahmann aschmahmann mentioned this pull request Nov 15, 2024
Merged
@hsanjuan
Copy link
Contributor

Reviewing this and probably merging into #641 ...

hsanjuan
hsanjuan reviewed Nov 18, 2024
View reviewed changes
blockservice/blockservice.go
Comment on lines -143 to -148
ses := grabSessionFromContext(ctx, bs)
if ses != nil {
return ses
}

return newSession(ctx, bs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why session-grabbing from context is removed... I remember old discussions about embedding sessions in contexts etc. but not sure if used...

@hsanjuan
Copy link
Contributor

This was more or less made part of #641

@hsanjuan hsanjuan closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsanjuan hsanjuan hsanjuan left review comments

@lidel lidel Awaiting requested review from lidel lidel is a code owner

Assignees

@gammazero gammazero

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gammazero @hsanjuan