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

move lotus mount, dag store etc from markets to lotus #6793

Merged
merged 7 commits into from
Jul 20, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jul 19, 2021

No description provided.

)

var log = logging.Logger("dagstore-wrapper")
var gcInterval = 5 * time.Minute
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want this to be configurable via config.yml. I can submit a PR to add a DAG Store configuration. Then that can take the place of MarketsDAGStoreConfig!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍
Suggest we do this as a separate PR

markets/dagstore/dagstorewrapper.go Outdated Show resolved Hide resolved
return nil, xerrors.Errorf("failed to create DAG store: %w", err)
}

ctx, cancel := context.WithCancel(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be constructed through DI likely. We should take a context.Config in the constructor, and then wire it in the node package to the DI context. I can help with that if you need advice on how to do it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Start and Stop hooks that call a new Start(ctx) method and the existing Close() method respectively

markets/dagstore/dagstorewrapper.go Outdated Show resolved Hide resolved
markets/dagstore/dagstorewrapper.go Outdated Show resolved Hide resolved
markets/dagstore/mount_api.go Outdated Show resolved Hide resolved
markets/dagstore/mount_api_test.go Outdated Show resolved Hide resolved
@@ -609,9 +609,10 @@ func StorageProvider(minerAddress dtypes.MinerAddress,
}

opt := storageimpl.CustomDealDecisionLogic(storageimpl.DealDeciderFunc(df))
shardMigrator := storageimpl.NewShardMigrator(address.Address(minerAddress), ds, dagStore, pieceStore, spn)
Copy link
Member

Choose a reason for hiding this comment

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

Can we move the shard migrator here too? It too is specific to Lotus, so I think it makes sense to move into the Lotus repo.

(Suggestion: submit another PR on top of this one to avoid bloating this one)

markets/dagstore/dagstorewrapper.go Outdated Show resolved Hide resolved
node/modules/storageminer.go Outdated Show resolved Hide resolved
@dirkmc dirkmc marked this pull request as ready for review July 20, 2021 13:08
@aarshkshah1992 aarshkshah1992 merged commit 44496af into feat/replace-multistore-carv2 Jul 20, 2021
@aarshkshah1992 aarshkshah1992 deleted the refactor/mv-lotus-mount branch July 20, 2021 15:03
@aarshkshah1992
Copy link
Contributor

@raulk Merging it as per today's discussion so we can start testing. Please continue leaving comments here and we'll make sure we address them.

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.

3 participants