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.

Sign up for GitHub

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

Jump to bottom

Persisting Storage Deals #1526

Merged
merged 1 commit into from
Jan 16, 2019
Merged

Persisting Storage Deals #1526

merged 1 commit into from
Jan 16, 2019

Conversation

lkowalick
Copy link
Contributor

@lkowalick lkowalick commented Dec 19, 2018 •
edited
Loading

Resolves #1446

We persist deals to the disk to allow the continuation of deals after
nodes are restarted.

Additionally, we add:

  • A default address option to init
  • The ability to start, stop, and restart a test daemon

@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch from 658ca35 to 048df61 Compare December 19, 2018 16:51
@rosalinekarr rosalinekarr force-pushed the feat/persist-deals-1446 branch from 6204fdf to 7c5a644 Compare December 21, 2018 16:23
@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch 3 times, most recently from 9e6b34a to 81d3361 Compare January 2, 2019 20:57
@rosalinekarr rosalinekarr force-pushed the feat/persist-deals-1446 branch from b5c0deb to aea99b5 Compare January 3, 2019 15:23
@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch 2 times, most recently from db915df to 8e69943 Compare January 3, 2019 21:38
lkowalick
lkowalick commented Jan 4, 2019
View reviewed changes
functional-tests/retrieval Outdated Show resolved Hide resolved
lkowalick
lkowalick commented Jan 4, 2019
View reviewed changes
protocol/storage/client.go Outdated Show resolved Hide resolved
@rosalinekarr rosalinekarr force-pushed the feat/persist-deals-1446 branch from afe35df to 93c5a5a Compare January 4, 2019 20:53
lkowalick
lkowalick commented Jan 4, 2019
View reviewed changes
node/node_test.go Outdated Show resolved Hide resolved
@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch 3 times, most recently from fe07d1c to b12bc5d Compare January 4, 2019 21:28
@lkowalick lkowalick requested review from whyrusleeping, ZenGround0, phritz and laser and removed request for whyrusleeping and laser January 4, 2019 21:28
@lkowalick lkowalick changed the title WIP WIP Persisting Storage Deals Persisting Storage Deals Jan 4, 2019
@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch from b12bc5d to 45c8593 Compare January 4, 2019 23:16
phritz
phritz suggested changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@phritz phritz left a comment

Choose a reason for hiding this comment

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

Nice, just one major request (lots of little ones).

Also: can you tell me the top 2-ish things that slowed you down on this? Was it a lot of learning curve or something else?

protocol/storage/client.go Outdated Show resolved Hide resolved
protocol/storage/client.go Outdated Show resolved Hide resolved
protocol/storage/client.go Outdated Show resolved Hide resolved
protocol/storage/client.go Outdated Show resolved Hide resolved
protocol/storage/miner.go Outdated Show resolved Hide resolved
protocol/storage/miner.go Outdated Show resolved Hide resolved
repo/fsrepo.go Outdated Show resolved Hide resolved
repo/fsrepo.go Outdated Show resolved Hide resolved
testhelpers/commands.go Outdated Show resolved Hide resolved
commands/client_daemon_test.go Show resolved Hide resolved
@lkowalick
Copy link
Contributor Author

@phritz This PR is ready with the following limitations:

  1. Both you and @ZenGround0 approve the PR
  2. Confirmation from @whyrusleeping that we adequately addressed his comments.
  3. Our new test is skipped since it will likely introduce more flakiness. The storage protocol flakiness is going to be handled immediately afterwards, which will include reducing the flakiness.
  4. Some of the error propagation is also not fully formed, but will also be a part of the above test protocol hardening.
  5. Namespacing the datastores will also be a reasonably high priority followup task.

@lkowalick lkowalick dismissed ZenGround0’s stale review January 15, 2019 15:34

Hopefully adequately addressed feedback

@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch from 3f4945d to 92d0078 Compare January 15, 2019 16:18
ZenGround0
ZenGround0 approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Approving because this PR doesn't make things worse, and we need to clear the slate to address the slew of issues referenced by #1561. @rkowalick has agreed to pick up that issue next as well as add comments to #1561 describing the nuances of why the storage market is low quality learned from this PR.

I think we should not skip tests, because the pain of flaky tests will help the team prioritizing getting the storage market to a reasonable quality.

@phritz
Copy link
Contributor

phritz commented Jan 15, 2019

FYI I had one blocking comment that was never fully resolved: filing an issue on refmt. Note that I think there is already an issue for this, found this referenced in the miner actor code: polydawn/refmt#35. I think we should ask @warpfork to fix this in refmt.

@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch from 92d0078 to 2f547e3 Compare January 15, 2019 20:08
@laser
Copy link
Contributor

laser commented Jan 15, 2019

FYI: The referenced refmt issue also impacts the miner actor's SectorCommitments map.

@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch 6 times, most recently from 7a6d5bc to 0c7dd90 Compare January 16, 2019 00:33
@rosalinekarr rosalinekarr force-pushed the feat/persist-deals-1446 branch from 0c7dd90 to 84ea467 Compare January 16, 2019 14:01
phritz
phritz approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@phritz phritz left a comment

Choose a reason for hiding this comment

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

please keep an eye out for increased flakiness from this work

api/impl/daemon.go
@@ -99,6 +99,14 @@ func (nd *nodeDaemon) Init(ctx context.Context, opts ...api.DaemonInitOpt) error
}
}

if cfg.DefaultAddress != (address.Address{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does https://github.com/filecoin-project/go-filecoin/blob/a3899d7ae872361940fc18dce6b57bbca9996270/plumbing/msg/sender.go#L123 need to change? should this code be using it?

protocol/storage/types.go Show resolved Hide resolved
@rosalinekarr
Deals Persistence
18f1ad4 
We persist deals to the disk to allow the continuation of deals after
nodes are restarted.

Additionally, we add:
- A default address option to init
- The ability to start, stop, and restart a test daemon

Co-authored-by: rkowalick <[email protected]>
@lkowalick lkowalick force-pushed the feat/persist-deals-1446 branch from 84ea467 to 18f1ad4 Compare January 16, 2019 22:23
@lkowalick lkowalick merged commit 701a55a into master Jan 16, 2019
@lkowalick lkowalick deleted the feat/persist-deals-1446 branch January 16, 2019 22:56
@lkowalick lkowalick mentioned this pull request Jan 16, 2019
Closed
lkowalick pushed a commit to filecoin-project/specs that referenced this pull request Jan 18, 2019
Update Deals Proposal to ProposalCid
da9e431 
A change in filecoin-project/venus#1526 renamed this field from
`Proposal` to `ProposalCid`
@lkowalick lkowalick mentioned this pull request Jan 18, 2019
Merged
@lkowalick lkowalick mentioned this pull request Feb 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosalinekarr rosalinekarr rosalinekarr left review comments

@whyrusleeping whyrusleeping whyrusleeping left review comments

@ZenGround0 ZenGround0 ZenGround0 approved these changes

@phritz phritz phritz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lkowalick @phritz @laser @whyrusleeping @ZenGround0 @rosalinekarr