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

Feat/snap deals storage #7615

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Feat/snap deals storage #7615

merged 3 commits into from
Nov 30, 2021

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented Nov 10, 2021

WIP introducing snap deals support for sector storage by adding a new file type.

Needs filecoin-project/specs-storage#17

@ZenGround0 ZenGround0 force-pushed the feat/snap-deals-storage branch from 24bc634 to 1f79db0 Compare November 10, 2021 18:53
@ZenGround0 ZenGround0 force-pushed the feat/snap-deals-storage branch 2 times, most recently from 2e37de1 to 797c15b Compare November 22, 2021 21:09
@ZenGround0 ZenGround0 changed the base branch from master to next November 23, 2021 21:37
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #7615 (40d16a8) into next (9d44893) will increase coverage by 0.05%.
The diff coverage is 54.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #7615      +/-   ##
==========================================
+ Coverage   39.50%   39.56%   +0.05%     
==========================================
  Files         646      646              
  Lines       68376    68582     +206     
==========================================
+ Hits        27015    27136     +121     
- Misses      36722    36772      +50     
- Partials     4639     4674      +35     
Impacted Files Coverage Δ
api/api_storage.go 0.00% <ø> (ø)
extern/sector-storage/ffiwrapper/verifier_cgo.go 75.32% <ø> (+2.59%) ⬆️
extern/sector-storage/manager_calltracker.go 62.55% <ø> (+4.84%) ⬆️
extern/sector-storage/mock/mock.go 62.03% <0.00%> (-2.95%) ⬇️
extern/sector-storage/sealtasks/task.go 81.81% <ø> (ø)
extern/sector-storage/storiface/worker.go 53.84% <ø> (ø)
extern/sector-storage/ffiwrapper/sealer_cgo.go 58.79% <39.58%> (-2.13%) ⬇️
extern/sector-storage/manager.go 63.06% <56.25%> (-1.88%) ⬇️
extern/sector-storage/worker_local.go 62.88% <72.72%> (+0.80%) ⬆️
extern/sector-storage/storiface/filetype.go 91.25% <100.00%> (+1.54%) ⬆️
... and 28 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 9d44893...40d16a8. Read the comment docs.

@ZenGround0 ZenGround0 force-pushed the feat/snap-deals-storage branch from 7ef1652 to 7d2b3f0 Compare November 29, 2021 15:24
@ZenGround0 ZenGround0 marked this pull request as ready for review November 29, 2021 16:45
@ZenGround0 ZenGround0 requested a review from a team as a code owner November 29, 2021 16:45
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Don't see much to complain about, looks pretty clean

Comment on lines 71 to 75
// for _, path := range t.StoragePaths {
// if err := os.RemoveAll(path.Path); err != nil {
// fmt.Println("Cleanup error:", err)
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Bring back before merging (could add an env var to turn this cleanup off)

Comment on lines +208 to +209
case "2k":
case "8M":
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing 2k?

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 should be covered by the default above

ctx, cancel := context.WithCancel(ctx)
defer cancel()

wk, wait, cancel, err := m.getWork(ctx, sealtasks.TTProveReplicaUpdate1, sector, sectorKey, newSealed, newUnsealed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, doesn't this cancel shadow the cancel above, and don't we end up calling the getWork cancel twice, and not calling context.WithCancel?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I realize we're doing the same above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the first cancel function is captured before shadowing by the first defer. I wasn't sure and made this example to get to this conclusion: https://go.dev/play/p/5IAnJuFg69N. I think that means this pattern is ok. Make sense to you?

@magik6k
Copy link
Contributor

magik6k commented Nov 30, 2021

(we also should bump minor worker/miner API versions (api/version.go) since we're adding new APIs, but do that after rebasing on latest master because conflicts)

@ZenGround0
Copy link
Contributor Author

(we also should bump minor worker/miner API versions (api/version.go) since we're adding new APIs, but do that after rebasing on latest master because conflicts)

Good call out. I don't know the workflow for rebasing next on master but will do this when that time comes.

@ZenGround0 ZenGround0 merged commit 91f6d4b into next Nov 30, 2021
@ZenGround0 ZenGround0 deleted the feat/snap-deals-storage branch November 30, 2021 19:18
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