review of dagstore + carv2 #596

raulk · 2021-08-03T10:39:22Z

consolidation of small packages related to blockstore management and filestores under package stores.
naming changes for more succinctness.
simplify blockstore trackers; revise naming.
storage provider: register ongoing CAR blockstores on restart; more obvious than the previous behaviour.

TODO

Refactoring migration.

codecov-commenter · 2021-08-03T18:58:10Z

Codecov Report

Merging #596 (7a479e7) into feat/wip-markets-dagstore (cf7e2a9) will increase coverage by 0.87%.
The diff coverage is 59.52%.

@@                      Coverage Diff                      @@
##           feat/wip-markets-dagstore     #596      +/-   ##
=============================================================
+ Coverage                      64.97%   65.83%   +0.87%     
=============================================================
  Files                             67       63       -4     
  Lines                           4649     4562      -87     
=============================================================
- Hits                            3020     3003      -17     
+ Misses                          1342     1268      -74     
- Partials                         287      291       +4

Impacted Files	Coverage Δ
storagemarket/events.go	`40.00% <ø> (ø)`
storagemarket/impl/client_environments.go	`0.00% <0.00%> (ø)`
storagemarket/impl/providerstates/provider_fsm.go	`73.73% <ø> (-0.22%)`	⬇️
storagemarket/impl/requestvalidation/common.go	`85.19% <ø> (-1.91%)`	⬇️
storagemarket/types.go	`85.37% <ø> (ø)`
stores/dagstore.go	`0.00% <0.00%> (ø)`
stores/error.go	`100.00% <ø> (ø)`
storagemarket/impl/provider_environments.go	`8.42% <12.50%> (-0.96%)`	⬇️
storagemarket/impl/client.go	`21.48% <40.00%> (-0.07%)`	⬇️
storagemarket/impl/provider.go	`27.92% <41.67%> (-0.52%)`	⬇️
... and 17 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 a44f98a...7a479e7. Read the comment docs.

raulk · 2021-08-03T21:15:27Z

storagemarket/impl/client.go

+		FastRetrieval:      params.FastRetrieval,
+		DealStages:         storagemarket.NewDealStages(),
+		CreationTime:       curTime(),
+		IndexedCAR:         params.IndexedCAR,


I changed the name of this field to describe function, and not type. I don't think this affects existing deals, because I believe that CBOR is positional, but would love a confirmation, since you've seen several upgrades through @dirkmc

I'm not 100% sure about that - maybe write a test to confirm?

It's gonna be hard writing a test for that :-( I'll hack something but it won't go in as a test.

raulk · 2021-08-03T21:16:06Z

storagemarket/impl/provider.go

@@ -263,7 +262,7 @@ func (p *Provider) receiveDeal(s network.StorageDealStream) error {
 		Ref:                proposal.Piece,
 		FastRetrieval:      proposal.FastRetrieval,
 		CreationTime:       curTime(),
-		CARv2FilePath:      carV2FilePath,
+		InboundCAR:         path,


Changed the field name. Idem as above @dirkmc

raulk · 2021-08-03T21:21:49Z

storagemarket/impl/provider.go

+	// re-track all deals for whom we still have a local blockstore.
+	for _, d := range deals {
+		if _, err := os.Stat(d.InboundCAR); err == nil && d.Ref != nil {
+			_, _ = p.stores.GetOrOpen(d.ProposalCid.String(), d.InboundCAR, d.Ref.Root)
+		}
+	}


We now load all in progress deals whose inbound CAR still exists on disk into the trackers. This reduces complexity of the former FinalizeReadWriteBlockstore on providerDealEnvironment, which used to accept the root CID and the path to reload the entry in the tracker if it didn't exist. It puzzled me quite a bit. I believe restoring state on startup is more correct.

Sounds right to me 👍

raulk · 2021-08-03T21:22:05Z

storagemarket/impl/provider_environments.go

-func (p *providerDealEnvironment) FinalizeReadWriteBlockstore(proposalCid cid.Cid, carPath string, rootCid cid.Cid) error {
-	bs, err := p.p.readWriteBlockStores.GetOrCreate(proposalCid.String(), carPath, rootCid)
+func (p *providerDealEnvironment) FinalizeBlockstore(proposalCid cid.Cid) error {
+	bs, _, err := p.p.stores.Get(proposalCid.String())


Here's what I was talking about above.

raulk · 2021-08-03T21:23:43Z

stores/filestore.go

@@ -0,0 +1,161 @@
+package stores


The previous filestorecaradapter package is now merged into stores.

dirkmc · 2021-08-04T08:37:47Z

storagemarket/impl/provider.go

+	// re-track all deals for whom we still have a local blockstore.
+	for _, d := range deals {
+		if _, err := os.Stat(d.InboundCAR); err == nil && d.Ref != nil {
+			_, _ = p.stores.GetOrOpen(d.ProposalCid.String(), d.InboundCAR, d.Ref.Root)
+		}
+	}


Sounds right to me 👍

storagemarket/impl/provider_environments.go

stores/rw_bstores.go

dirkmc · 2021-08-04T08:54:10Z

Looks like you need to run make prepare-pr

raulk · 2021-08-04T12:03:29Z

The docs-check CI target is broken in the CircleCI config, nothing to do with this PR.

general consolidation and cleanup.

f122745

raulk force-pushed the raulk/review-dagstore branch from 268b814 to f122745 Compare August 3, 2021 10:40

raulk added 7 commits August 3, 2021 13:14

move DAGStoreWrapper to stores package.

3d14ff2

use more succinct variable names.

a12848e

simplify blockstore trackers; clean up naming.

2b0ee7c

provider: re-register blockstores on restart.

3fd257a

rename shardregistration to shard_migrator.

64734de

fix lint.

439a9ac

fix test compilation.

1631c88

re-add stores.Read{Only,Write}Filestore().

4d3607a

raulk commented Aug 3, 2021

View reviewed changes

stores/filestore.go

@@ -0,0 +1,161 @@

package stores

Copy link

Member Author

raulk Aug 3, 2021

Choose a reason for hiding this comment

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

The previous filestorecaradapter package is now merged into stores.

raulk requested a review from dirkmc August 3, 2021 21:25

dirkmc reviewed Aug 4, 2021

View reviewed changes

raulk added 4 commits August 4, 2021 12:31

address comments.

79ee3ff

wrap errors.

1867beb

quieten goimports.

7a479e7

run prepare-pr.

2a38ade

raulk marked this pull request as ready for review August 4, 2021 12:03

raulk merged commit cdd492e into feat/wip-markets-dagstore Aug 4, 2021

raulk deleted the raulk/review-dagstore branch August 4, 2021 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review of dagstore + carv2 #596

review of dagstore + carv2 #596

raulk commented Aug 3, 2021 •

edited

codecov-commenter commented Aug 3, 2021 •

edited

raulk Aug 3, 2021

dirkmc Aug 4, 2021

raulk Aug 4, 2021

raulk Aug 3, 2021

raulk Aug 3, 2021

dirkmc Aug 4, 2021

raulk Aug 3, 2021

raulk Aug 3, 2021

dirkmc Aug 4, 2021

dirkmc commented Aug 4, 2021

raulk commented Aug 4, 2021

review of dagstore + carv2 #596

review of dagstore + carv2 #596

Conversation

raulk commented Aug 3, 2021 • edited

TODO

codecov-commenter commented Aug 3, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc commented Aug 4, 2021

raulk commented Aug 4, 2021

raulk commented Aug 3, 2021 •

edited

codecov-commenter commented Aug 3, 2021 •

edited