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

Update to graphsync to v0.10.0, enable seperate storage and retrieval transfer limits #7405

Merged
merged 6 commits into from
Oct 5, 2021

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Sep 30, 2021

Goals

Update go-graphsync v0.10.0

Add support for seperate transfer limits across storage and retrieval, aiming to provide initial solutions for #7276

Blockers:

  • update to final go-graphsync v0.10.0 (pending use and validation of graphsync/do-not-send-first-blocks in go-data-transfer)
  • verify no issues with renaming SimultaneousTransfers, or clear upgrade path

Implementation

  • update go-graphsync to v0.10.0 with outgoing request limits
  • rename configuration SimultaneousTransfers -> SimultaneousTransfersForStorage, SimultaneousTransfersForRetrieval
  • configure request limists accordingly for graphsync (note these are opposite between client + miner see table below)

For discussion

Mapping role and deal type to graphsync request type:

Role DealType GS Request Type
Client Storage Incoming
Client Retrieval Outgoing
Provider Storage Outgoing
Provider Retrieval Incoming

@hannahhoward hannahhoward requested a review from a team as a code owner September 30, 2021 01:59
@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #7405 (f60692c) into master (fc10281) will increase coverage by 0.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7405      +/-   ##
==========================================
+ Coverage   39.54%   39.59%   +0.05%     
==========================================
  Files         616      616              
  Lines       65295    65319      +24     
==========================================
+ Hits        25818    25863      +45     
+ Misses      34980    34968      -12     
+ Partials     4497     4488       -9     
Impacted Files Coverage Δ
node/config/def.go 97.38% <71.42%> (-1.27%) ⬇️
node/impl/client/client.go 40.73% <83.33%> (+0.26%) ⬆️
node/builder_chain.go 100.00% <100.00%> (ø)
node/builder_miner.go 94.85% <100.00%> (ø)
node/modules/client.go 74.03% <100.00%> (ø)
node/modules/graphsync.go 73.07% <100.00%> (+9.91%) ⬆️
node/modules/storageminer.go 62.93% <100.00%> (+0.43%) ⬆️
extern/sector-storage/sched_resources.go 75.00% <0.00%> (-9.38%) ⬇️
chain/actors/builtin/paych/paych.go 16.88% <0.00%> (-5.20%) ⬇️
chain/exchange/peer_tracker.go 66.66% <0.00%> (-4.31%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc10281...f60692c. Read the comment docs.

@magik6k
Copy link
Contributor

magik6k commented Oct 1, 2021

(needs a rebase + make docsgen-cli because these is some new config-related docgen in master)

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

@jennijuju: it'd be worth calling out in release notes that SimultaneousTransfers stops working entirely here and there's no migration or repurposing. If you were using that then you need to update to have both SimultaneousTransfersForStorage and SimultaneousTransfersForRetrieval.

@rvagg rvagg force-pushed the feat/update-graphsync-0.10.0 branch from b41a397 to 368d72e Compare October 5, 2021 03:15
@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

Rebased to current master. We've lost a bit of codecov here and it's red, but it comes back up via #7427 which is on top of this branch.

@jennijuju
Copy link
Member

jennijuju commented Oct 5, 2021

@jennijuju: it'd be worth calling out in release notes that SimultaneousTransfers stops working entirely here and there's no migration or repurposing. If you were using that then you need to update to have both SimultaneousTransfersForStorage and SimultaneousTransfersForRetrieval.

Ack! Should we recommend SP to finish/stop all transfers before updating then?

@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

Ack! Should we recommend SP to finish/stop all transfers before updating then?

Oh, I'm not sure about that; I suppose it won't matter, the upgrade will bring better restart functionality as well as these limits, but the change in config options shouldn't be a major change in behaviour, just an annoying config change if it's been configured. Maybe @dirkmc can chime in on this.

@dirkmc
Copy link
Contributor

dirkmc commented Oct 5, 2021

I don't think we need to explicitly ask storage providers to stop data transfers before upgrading

@jennijuju
Copy link
Member

I don't think we need to explicitly ask storage providers to stop data transfers before upgrading

So when they restart the node with X ongoing transfer, where X > SimultaneousTransfersForStorage, new deals will just be rejected and the previously ongoing transfer can be resumed?

@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

@jennijuju I don't know the answer to that but I'm assuming that would be correct. Old default is 20, new default is 20, so the only surprises here would be if an SP has changed the number on their end, they would need to set both SimultaneousTransfersForStorage and SimultaneousTransfersForRetrieval to that number to achieve the same effect as their old config.

Limit maximum number of DAG links to traverse
@jennijuju jennijuju merged commit cbb147d into master Oct 5, 2021
@jennijuju jennijuju deleted the feat/update-graphsync-0.10.0 branch October 5, 2021 15:51
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.

Add SimultaneousTransfers limitation for incoming data transfers from Storage Deals
5 participants