Index publishing work #673

aarshkshah1992 · 2022-01-20T12:04:49Z

No description provided.

…rovider (#647) * reference prov integration * Apply suggestions from code review Co-authored-by: dirkmc <[email protected]> * move provider callback to start * fix: lint Co-authored-by: dirkmc <[email protected]>

…o-fil-markets into feat/retrieve-any-cid

* add fullnodeApi to Provider * add idxProvHost * connect to full node before making announcement * fix compilation Co-authored-by: Anton Evangelatov <[email protected]>

* use NetAddrListener iface * fix compilation * fix imports Co-authored-by: Aarsh Shah <[email protected]>

codecov-commenter · 2022-02-02T15:13:10Z

Codecov Report

Merging #673 (59b99bf) into master (5e87024) will decrease coverage by 1.05%.
The diff coverage is 30.15%.

❗ Current head 59b99bf differs from pull request most recent head a6e18af. Consider uploading reports for the commit a6e18af to get more accurate results

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
- Coverage   59.68%   58.63%   -1.04%     
==========================================
  Files          64       64              
  Lines        5262     5356      +94     
==========================================
  Hits         3140     3140              
- Misses       1790     1878      +88     
- Partials      332      338       +6

Impacted Files	Coverage Δ
storagemarket/impl/provider_environments.go	`9.18% <0.00%> (-1.35%)`	⬇️
stores/dagstore.go	`0.00% <ø> (ø)`
stores/filestore.go	`54.55% <ø> (ø)`
storagemarket/impl/provider.go	`23.12% <12.33%> (-2.54%)`	⬇️
...oragemarket/impl/providerstates/provider_states.go	`84.99% <62.50%> (-0.58%)`	⬇️
retrievalmarket/impl/provider.go	`69.32% <65.00%> (-1.41%)`	⬇️
retrievalmarket/impl/provider_environments.go	`69.05% <100.00%> (+2.97%)`	⬆️
retrievalmarket/impl/client.go	`67.22% <0.00%> (-5.39%)`	⬇️

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 f746f85...a6e18af. Read the comment docs.

* remove libp2p host connect in favour of mesh creator interface * update test harness

Upgrade to the latest `index-provider` which includes two main changes: 1. update to go-legs message format 2. cache eviction bug fix

Upgrade to `index-provider` `v0.3.0`

…-dont-return log failure to connect to full node without returning it

Update to the head of the PR that introduces indexing integration in `go-fil-markets` so that failure to connect to full node is logged only instead of crashing markets process. Relates to: - filecoin-project/go-fil-markets#673 - ipni/index-provider#177

Depend on a concrete tagged release of dagstore that contains fixes to dagstore repo bloat.

Update indexing dependencies to latest along with go-fil-markets dependency, to the head of: - filecoin-project/go-fil-markets#673

hannahhoward

My biggest meta comment is you should remove:

CIDInfo entirely from the PieceStore -- it's unused
Remove the whole "MetadataPath" if that machinery still exists -- it's also unused

It's important to make clear to someone reading the code that the single source of truth on which payloadCIDs are in which pieces is now in the DagStore only.

hannahhoward · 2022-02-25T00:23:35Z

retrievalmarket/impl/client_test.go

@@ -225,6 +225,7 @@ func TestClient_FindProviders(t *testing.T) {
 // retrieval deal for the same payload CID with the same peer as an existing
 // active deal
 func TestClient_DuplicateRetrieve(t *testing.T) {
+	t.Skip("flaky test")


Did it become more flaky in this PR?

Yes -- then let's figure that out

No -- I would not throw in a skip as part of a feature PR -- i'd just make an issue and if we can't tackle it now, let's make a seperate PR to add a skip

Issue at TestClient_DuplicateRetrieve is flaky #683.

hannahhoward · 2022-02-25T00:36:44Z

retrievalmarket/impl/provider.go

@@ -448,6 +461,65 @@ func (p *Provider) pieceInUnsealedSector(ctx context.Context, pieceInfo piecesto
 	return false
 }

+func (p *Provider) storageDealsForPiece(clientSpecificPiece bool, payloadCID cid.Cid, pieceInfo piecestore.PieceInfo) ([]abi.DealID, error) {


This is an existing problem (not introduced by this PR) but this method has a weird logic to it -

if the first parameter is true, then the third parameter is used, the second parameter unused

if the first parameter is false, the second parameter is used, the third parameter unused

To me this says: make it two methods -- and also, consider the interrelated logic from the the HandleQueryStream/getPieceInfoFromCid and see if there's a way to simply.

Definitely non-blocking, just noting.

hannahhoward · 2022-02-25T00:39:26Z

storagemarket/impl/provider.go

@@ -177,6 +190,25 @@ func (p *Provider) Start(ctx context.Context) error {
 			log.Error(err.Error())
 		}
 	}()
+
+	// connect the index provider node with the full node and protect that connection


This feels like a very odd concern to put in go-fil-markets... maybe I don't know the context? I would expect it to live in the Lotus markets process. (or later Boost)

fil-markets manages the libp2p host though, i guess?

We do wanna make sure that we do this everytime before we announce a deal in markets just to be sure we have a connection to the daemon before making the announcement.

But yeah, will move this to Boost when we integrate the index provider in Boost.

hannahhoward · 2022-02-25T00:43:24Z

storagemarket/impl/providerstates/provider_states.go

+	// announce the deal to the network indexer
+	annCid, err := environment.AnnounceIndex(ctx.Context(), deal)
+	if err != nil {
+		log.Errorw("failed to announce index via reference provider", "proposalCid", deal.ProposalCid, "err", err)


Should the fact that an error occurred in publish be saved to the deal state? Other than logs, how do I know which of my deals got announced successfully? Do I always want AnnounceAllDealsToIndex() to announce everything?

@hannahhoward The next time a deal gets announced, because of the IPLD magic in the index provider, it will also announce all the previous un-announced deal when the network indexer asks for the latest.

@aarshkshah1992 not necessarily.
If the error happens because the advertisement failed to persist then that content will never get advertised.
Looking at the code, such errors can happen before call to NotifyPut e.g. in metadata encoding on markets side or inside the provider before persistence.

The case you mentioned above will only work if the new advertisement is added to the internal IPLD chain of ads but not announced to the network due to say gossipsub error.

Also, AnnounceAllDealsToIndex is idempotent because the Index provider is idempotent. It wont actually have to serve indices for deals for which it has already served indices.

aarshkshah1992 · 2022-02-28T15:03:30Z

My biggest meta comment is you should remove:

CIDInfo entirely from the PieceStore -- it's unused

Remove the whole "MetadataPath" if that machinery still exists -- it's also unused

It's not really a part of this work. I started doing thus but looks like a bigger refactor given that it's very pervasive in all the tests too. I've created #684 to track this and we can do this in a follow up PR.

It's important to make clear to someone reading the code that the single source of truth on which payloadCIDs are in which pieces is now in the DagStore only.

Have added a comment for this.

- Add comment to clarify the reason for loop in testkit - Trim common prefix in state printed in CLI commands for better readability - Upgrade to a tagged release of `go-fil-markets` that includes indexing work; see: filecoin-project/go-fil-markets#673 - Fix typo in CLI usage. - Add comments to note that it is safe to use fx `OnStart` context when starting the provider engine. - Fix string concatenation in error message formatting.

dirkmc and others added 23 commits September 9, 2021 11:51

feat: retrieve by any CID (not just root CID)

566b389

feat: fix tests

5a1a1b5

fix: TestHandleQueryStream

209b432

fix: integration tests

7fba642

fix merge

a85e8b3

fix conflict

c6619ed

update dagstore and disable flaky

7f70337

refactor: use index-provider instead of indexer-reference-provider

17c3a6c

update to latest data-transfer and index-provider

8713155

announce all deals to the indexer

31e067c

Merge remote-tracking branch 'origin/master' into feat/retrieve-any-cid

383dfa8

update go mods

e251d80

update deps

7165da0

lint: fix imports

8547a2d

update dagstore to tagged ver

7f49c96

Merge branch 'feat/retrieve-any-cid' of github.com:filecoin-project/g…

d1271cd

…o-fil-markets into feat/retrieve-any-cid

update provider

068e7f3

update deps

e0faa0e

Merge remote-tracking branch 'origin/master' into feat/retrieve-any-cid

9f56617

update deps

9160420

log advertisement cid for announcement and update deps

e8772da

update deps

eb2fe1f

aarshkshah1992 force-pushed the feat/retrieve-any-cid branch from 6d24ebc to eb2fe1f Compare January 21, 2022 14:53

aarshkshah1992 and others added 6 commits January 26, 2022 10:54

more logging for announcement

3d2e3c8

better logic to announce all deals to network indexer

06ec150

Connect index provider host to full node (#675)

7a2e429

* add fullnodeApi to Provider * add idxProvHost * connect to full node before making announcement * fix compilation Co-authored-by: Anton Evangelatov <[email protected]>

fix imports

c1c9ba8

log connection

93e4c64

use NetAddrListener iface (#676)

ea0c8f1

* use NetAddrListener iface * fix compilation * fix imports Co-authored-by: Aarsh Shah <[email protected]>

nonsense and others added 5 commits February 3, 2022 19:24

remove libp2p host connect in favour of mesh creator interface (#678)

8790cca

* remove libp2p host connect in favour of mesh creator interface * update test harness

Upgrade to index-provider v.0.3.0

d26bd86

Upgrade to the latest `index-provider` which includes two main changes: 1. update to go-legs message format 2. cache eviction bug fix

Merge pull request #680 from filecoin-project/masih/dep-up-idx-prov-v030

44fb837

Upgrade to `index-provider` `v0.3.0`

log failure to connect to full node without returning it

ddc832f

Merge pull request #681 from filecoin-project/nonsense/only-log-error…

a0db7a7

…-dont-return log failure to connect to full node without returning it

masih mentioned this pull request Feb 15, 2022

Update to the head of go-fil-markets#673 filecoin-project/lotus#8097

Merged

aarshkshah1992 and others added 4 commits February 18, 2022 13:03

update dagstore

0f62c29

Use the latest tagged release for dagstore dependency

13b70d3

Depend on a concrete tagged release of dagstore that contains fixes to dagstore repo bloat.

Merge branch 'master' into feat/retrieve-any-cid

59b99bf

Upgrade to latest indexing dependencies

a6e18af

masih added a commit to filecoin-project/lotus that referenced this pull request Feb 23, 2022

Update indexing and markets dependencies

c3b406c

Update indexing dependencies to latest along with go-fil-markets dependency, to the head of: - filecoin-project/go-fil-markets#673

Run go mod tidy

2460e15

dirkmc approved these changes Feb 23, 2022

View reviewed changes

hannahhoward reviewed Feb 25, 2022

View reviewed changes

masih mentioned this pull request Mar 1, 2022

Content may never get advertised on failure to persist #685

Open

aarshkshah1992 merged commit 871e17d into master Mar 1, 2022

masih mentioned this pull request Mar 2, 2022

feat: markets: Integrate index ingest protocol and retrieve by any CID filecoin-project/lotus#7313

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index publishing work #673

Index publishing work #673

aarshkshah1992 commented Jan 20, 2022

codecov-commenter commented Feb 2, 2022 •

edited

Loading

hannahhoward left a comment

hannahhoward Feb 25, 2022

aarshkshah1992 Feb 28, 2022

hannahhoward Feb 25, 2022

hannahhoward Feb 25, 2022

willscott Feb 25, 2022

aarshkshah1992 Feb 28, 2022

hannahhoward Feb 25, 2022

aarshkshah1992 Feb 28, 2022

masih Feb 28, 2022 •

edited

Loading

aarshkshah1992 Feb 28, 2022

masih Mar 1, 2022

aarshkshah1992 commented Feb 28, 2022

Index publishing work #673

Index publishing work #673

Conversation

aarshkshah1992 commented Jan 20, 2022

codecov-commenter commented Feb 2, 2022 • edited Loading

Codecov Report

hannahhoward left a 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

masih Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Feb 28, 2022

codecov-commenter commented Feb 2, 2022 •

edited

Loading

masih Feb 28, 2022 •

edited

Loading