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

use addresses from miner info for connecting to miners #290

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

whyrusleeping
Copy link
Member

Not quite complete as i havent gotten the tests to pass yet, but this is most of the work.

Copy link
Collaborator

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

ncie

@whyrusleeping
Copy link
Member Author

@hannahhoward i'm not exactly sure what the right way to get tests passing here is, any thoughts?

@hannahhoward
Copy link
Collaborator

hannahhoward commented Jun 24, 2020

@whyrusleeping submitted PR to this branch to get tests to pass. Note: this does not make the integration tests actually test the functionality you've implemented. To do that you'd need to actually configure functionality to make a real second peer id (on libp2p mocknet) that the miner could listen on.

@hannahhoward
Copy link
Collaborator

hannahhoward commented Jun 24, 2020

Another question: in this scenario, is the miner running seperate lotus-storage-miner processes with a seperate store for tracking deal progress? If not there are some potential complications: when a storage provider is initiatilzed, it attempts to restart processing of deals in progress when it was shut down. If they share the same store, I'm not sure what happens-- one process might attempt to continue with another processes deals. So we may need to add a parameter to StorageProvider.Start that specifies whether or not to restart deals -- and only make that value true on one of the lotus-storage-miner processes.

@ribasushi
Copy link
Contributor

@whyrusleeping @magik6k this is somewhat blocking us to test an end-ot-end offline deal with dumbodrop. Is there something preventing a merge this week?

/cc @mikeal

@hannahhoward
Copy link
Collaborator

@ribasushi I was waiting for @whyrusleeping to merge #291 which gets test passing. I am going to assume he's got other things happening and I'll take this home today.

whyrusleeping and others added 3 commits June 26, 2020 14:37
Provide a real response to GetMinerInfo so that integration tests pass
fix problems from rebase and add a few doc things
@hannahhoward hannahhoward force-pushed the feat/use-miner-info-addrs branch from d2e4dda to 34adcc4 Compare June 26, 2020 21:44
@hannahhoward
Copy link
Collaborator

@ribasushi this is ready for merge now from my point of view, but I don't know if you want to get confirmation from @whyrusleeping or @magik6k

@hannahhoward
Copy link
Collaborator

(I went ahead and did the rebase)

@codecov-commenter
Copy link

Codecov Report

Merging #290 into master will decrease coverage by 0.22%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
- Coverage   61.77%   61.55%   -0.21%     
==========================================
  Files          41       41              
  Lines        2563     2572       +9     
==========================================
  Hits         1583     1583              
- Misses        852      861       +9     
  Partials      128      128              
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <0.00%> (ø)
storagemarket/network/libp2p_impl.go 77.97% <0.00%> (-2.73%) ⬇️
storagemarket/types.go 0.00% <ø> (ø)

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 ecce3a1...34adcc4. Read the comment docs.

@hannahhoward hannahhoward merged commit 2da69d9 into master Jun 29, 2020
hannahhoward added a commit that referenced this pull request Jun 29, 2020
* use addresses from miner info for connecting to miners

* fix(storagemarket): fix integration test harness (#291)

Provide a real response to GetMinerInfo so that integration tests pass

* fix(storagemarket): fix rebase issues

fix problems from rebase and add a few doc things

Co-authored-by: Hannah Howard <[email protected]>
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.

6 participants