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

sealing: DataCid on workers #8557

Merged
merged 5 commits into from
Apr 28, 2022
Merged

sealing: DataCid on workers #8557

merged 5 commits into from
Apr 28, 2022

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Apr 26, 2022

Depends on filecoin-project/specs-storage#26

This PR adds a ComputeDataCid which will use add-piece workers to compute piece CIDs. This is mostly useful for scaling boost.

Followups:

  • lotus-miner command to compute CommP of a file/url
  • Integrate into fil-markets (maybe)

@magik6k magik6k requested a review from a team as a code owner April 26, 2022 19:41
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #8557 (43436f7) into master (48ffb15) will decrease coverage by 7.55%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8557      +/-   ##
==========================================
- Coverage   40.86%   33.30%   -7.56%     
==========================================
  Files         686      670      -16     
  Lines       75582    74780     -802     
==========================================
- Hits        30883    24903    -5980     
- Misses      39374    45021    +5647     
+ Partials     5325     4856     -469     
Impacted Files Coverage Δ
api/api_storage.go 0.00% <ø> (ø)
cmd/lotus-worker/main.go 0.00% <0.00%> (ø)
cmd/lotus-worker/tasks.go 27.27% <ø> (ø)
extern/sector-storage/mock/mock.go 45.13% <0.00%> (-15.07%) ⬇️
extern/sector-storage/sealtasks/task.go 47.36% <ø> (-21.06%) ⬇️
extern/sector-storage/storiface/worker.go 56.00% <ø> (-20.00%) ⬇️
extern/sector-storage/worker_local.go 53.10% <71.42%> (-7.42%) ⬇️
extern/sector-storage/ffiwrapper/sealer_cgo.go 44.96% <73.97%> (-13.83%) ⬇️
extern/sector-storage/manager.go 54.48% <87.50%> (-6.98%) ⬇️
extern/sector-storage/storiface/resources.go 58.20% <100.00%> (-17.55%) ⬇️
... and 223 more

extern/sector-storage/ffiwrapper/sealer_cgo.go Outdated Show resolved Hide resolved
extern/sector-storage/ffiwrapper/sealer_cgo.go Outdated Show resolved Hide resolved
cid.Cid
error
}, 1)
pbuf := <-throttle
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get the buffer from the throttle channel at the top of this for loop? That way you could read directly into the buffer instead of reading to an intermediate buffer and copying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but reading before throttling should be (very slightly) faster / more efficient, as you're not waiting with some threads for the read to complete - the data already sits in a buffer

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense assuming that the io buffer is smaller than chunk.Unpadded(), which I think is correct 👍

itests/worker_test.go Show resolved Hide resolved
@magik6k magik6k force-pushed the feat/worker-commp branch from 1815108 to 779e923 Compare April 27, 2022 18:44
@magik6k
Copy link
Contributor Author

magik6k commented Apr 27, 2022

So the itest with the bigger buffer is really flaky, with something weird in rpcenc happening:

2022-04-27T20:46:10.614+0200	WARN	rpc	go-jsonrpc@v0.1.5/handler.go:279	error in RPC call to 'Filecoin.ComputeDataCid': storage call error 0: %!w(pr read error: http: invalid Read on closed Body [Hostname: ukaszs-MacBook-Pro.local])
    worker_test.go:60: 
        	Error Trace:	worker_test.go:60
        	Error:      	Received unexpected error:
        	            	storage call error 0: %!w(pr read error: http: invalid Read on closed Body [Hostname: ukaszs-MacBook-Pro.local])
        	Test:       	TestWorkerDataCid
    blockminer.go:326: shutting down mining

Which is weird, because I don't think we've ever saw this in AddPiece (but we've also never tested it in itests with a worker + bigger bit of data); Something about how we handle responses in rpcenc is probably racy.

Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

It seems like the rpcenc error is unrelated to this change, so I'm approving 👍

Seems like it's worth looking into the rpcenc error as a follow-up

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.

2 participants