Move to an explicit piecestore and explicit unsealing. #54

hannahhoward · 2020-01-22T23:33:36Z

Goals

Reduce the node adapter interfaces so they can be satisfied by both go-filecoin and lotus, while also fully supporting CAR files and seperating interface boundaries

Implementation

Use a piece info state data base to track connections between CommP & deals
Wait for deal sealing in provider side then store sector info about piece
The CID Info would eventually be generated when writing the CAR
Add unsealing explicitly to the retrieval side and add tests

Outlines node interface changes and relevant code to remove the need for the node to track information about pieces

When retrieving, manually unseal as neccesary based on piece CID, removing need for node implementors to maintain transparently sealed blockstore

Initial implementation of the piecestore - based on statestore

codecov-io · 2020-01-22T23:47:16Z

Codecov Report

Merging #54 into master will increase coverage by 1.4%.
The diff coverage is 47.06%.

@@            Coverage Diff            @@
##           master      #54     +/-   ##
=========================================
+ Coverage   32.43%   33.82%   +1.4%     
=========================================
  Files          33       36      +3     
  Lines        2350     2608    +258     
=========================================
+ Hits          762      882    +120     
- Misses       1514     1590     +76     
- Partials       74      136     +62

Impacted Files	Coverage Δ
retrievalmarket/impl/client.go	`69.86% <ø> (ø)`	⬆️
retrievalmarket/network/libp2p_impl.go	`61.77% <ø> (ø)`	⬆️
retrievalmarket/network/query_stream.go	`66.67% <ø> (ø)`	⬆️
retrievalmarket/impl/blockio/reader.go	`71.43% <ø> (ø)`	⬆️
storagemarket/impl/provider_states.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/provider.go	`0% <0%> (ø)`	⬆️
...ievalmarket/impl/providerstates/provider_states.go	`94.32% <100%> (ø)`	⬆️
piecestore/types_cbor_gen.go	`29.45% <29.45%> (ø)`
retrievalmarket/impl/provider.go	`72.36% <76.48%> (+0.81%)`	⬆️
piecestore/piecestore.go	`86.37% <86.37%> (ø)`
... and 4 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 c44b6fe...5d352f3. Read the comment docs.

ingar

Some non-blocking comments... 👍

ingar · 2020-01-23T00:11:58Z

piecestore/piecestore.go

+}
+
+func (ps *pieceStore) AddDealForPiece(pieceCID []byte, dealInfo DealInfo) error {
+	// Do we need to de-dupe or anything here?


This comment is wondering about whether adding duplicate DealInfo (or BlockInfo below) is necessarily bad or not. Since it's a local store maybe it's not a big deal.

ingar · 2020-01-23T00:18:55Z

piecestore/types.go

+	HasBlockInfo(pieceCID []byte) (bool, error)
+	HasDealInfo(pieceCID []byte) (bool, error)


Are these going to be used in another context? They are unused here -- or would they be more useful if they didn't return an error?

hannahhoward · 2020-01-23T00:59:10Z

@ingar I'm gonna mess with this interface some more in my next PR, cause I realize we actually are gonna have to change some more things... :( so let's not block on it for now.

shannonwells

All the suggestions I have are super minor. Your call

shannonwells · 2020-01-23T00:51:18Z

piecestore/piecestore_test.go

+	assert.NoError(t, err)
+	blockInfos := []piecestore.BlockInfo{{testCid, 42, 43}}
+
+	err = ps.AddBlockInfosToPiece(pieceCid, blockInfos)


this should really be broken up into test cases for test hygiene and easier auditing.

yes, this is gonna get changed a bunch shortly so I will hold off for now

shannonwells · 2020-01-23T01:08:41Z

retrievalmarket/impl/testnodes/test_retrieval_client_node.go

@@ -10,6 +10,8 @@ import (
 	"github.com/filecoin-project/go-fil-markets/shared/types"
 )

+// TestRetrievalClientNode is a node adapter for a retrieval client whose responses


🙇‍♀ for clarifying comments

retrievalmarket/impl/blockunsealing/blockunsealing.go

shannonwells · 2020-01-23T01:38:56Z

retrievalmarket/impl/blockunsealing/blockunsealing_test.go

+)
+
+func TestNewLoaderWithUnsealing(t *testing.T) {
+	ctx := context.Background()


I don't really understand at this time how IPLD trees work so I can't review this adequately.

shannonwells · 2020-01-23T01:40:54Z

retrievalmarket/impl/blockunsealing/blockunsealing_test.go

+
+			// attemp to load again, will not unseal, will fail
+			_, err = loaderWithUnsealing.Load(testdata.MiddleMapNodeLnk, ipld.LinkContext{})
+			require.Error(t, err)


I'd feel better with require/assert.EqualError to check that the error is what you expected.

yea so, this isn't the best test, but the error is actually the same as if it weren't in the blockstore --

if it unsealed a second time, it would work, cause it would put the deleted block back in the block store.

so it's not a specific error -- it just doesn't attempt another unseal -- and then fails reading from the blockstore as a result

retrievalmarket/impl/clientstates/client_states.go

shannonwells · 2020-01-23T01:44:56Z

retrievalmarket/impl/provider.go


-	if err == nil {
+	if err == nil && len(pieceInfo.Deals) > 0 {
 		answer.Status = retrievalmarket.QueryResponseAvailable
 		// TODO: get price, look for already unsealed ref to reduce work


Do these TODOs need to be captured in issues?

nah that's copied code that should get deleted :)

actually i'm not sure will take another look

shannonwells · 2020-01-23T01:51:42Z

retrievalmarket/impl/integration_test.go

-		providerNode.ExpectPiece(piece, expectedQR.Size*uint64(i+1))
+		pieceStore.ExpectPiece(piece, piecestore.PieceInfo{
+			Deals: []piecestore.DealInfo{
+				piecestore.DealInfo{


the inner piecestore.DealInfo is redundant. only worth changing if you're touching the file IMO

Fix todos, import orders, extraneous typing

hannahhoward and others added 4 commits January 22, 2020 13:17

feat(piecestore): design piecestore for metadata

dffcd7c

Outlines node interface changes and relevant code to remove the need for the node to track information about pieces

feat(retrieval): add unsealing explicitly

ca5a9cd

When retrieving, manually unseal as neccesary based on piece CID, removing need for node implementors to maintain transparently sealed blockstore

feat(piecestore): initial implementation

36a0bda

Initial implementation of the piecestore - based on statestore

style(retrievalmarket): cleanup lint errors

f802fe3

hannahhoward requested review from shannonwells and ingar January 22, 2020 23:40

ingar approved these changes Jan 23, 2020

View reviewed changes

shannonwells approved these changes Jan 23, 2020

View reviewed changes

refactor(retrievalmarket): cleanups

5d352f3

Fix todos, import orders, extraneous typing

hannahhoward merged commit 77e66fe into master Jan 23, 2020

hannahhoward deleted the feat/maintain-piece-references branch April 30, 2020 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to an explicit piecestore and explicit unsealing. #54

Move to an explicit piecestore and explicit unsealing. #54

hannahhoward commented Jan 22, 2020

codecov-io commented Jan 22, 2020 •

edited

ingar left a comment

ingar Jan 23, 2020

ingar Jan 23, 2020

hannahhoward commented Jan 23, 2020

shannonwells left a comment •

edited

shannonwells Jan 23, 2020

hannahhoward Jan 23, 2020

shannonwells Jan 23, 2020

shannonwells Jan 23, 2020

shannonwells Jan 23, 2020

hannahhoward Jan 23, 2020

shannonwells Jan 23, 2020

hannahhoward Jan 23, 2020

hannahhoward Jan 23, 2020

shannonwells Jan 23, 2020

		HasBlockInfo(pieceCID []byte) (bool, error)
		HasDealInfo(pieceCID []byte) (bool, error)

Move to an explicit piecestore and explicit unsealing. #54

Move to an explicit piecestore and explicit unsealing. #54

Conversation

hannahhoward commented Jan 22, 2020

Goals

Implementation

codecov-io commented Jan 22, 2020 • edited

Codecov Report

ingar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhoward commented Jan 23, 2020

shannonwells left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 22, 2020 •

edited

shannonwells left a comment •

edited