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

Miner should not dial client on restart #5210

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Dec 16, 2020

Fixes #4991 #5211 #5224 #5268 #5038

  • Error out if there are too many consecutive restarts with no data transferred limit consecutive restarts with no data transfer go-data-transfer#129
  • Assess feasibility of writing an integration test / testground test
  • Manual test
    It wasn't a small task to write an integration or testground test, so this should be tested manually:
    • Bring up a client and miner
    • Start transferring a large file from client to miner
    • While the file is transferring, restart the miner
    • Ensure that the file completes transferring and the deal moves to the sealing state

@dirkmc dirkmc marked this pull request as draft December 16, 2020 15:49
@magik6k
Copy link
Contributor

magik6k commented Dec 16, 2020

(assuming that there's some good reason that this is a draft)

@dirkmc
Copy link
Contributor Author

dirkmc commented Dec 17, 2020

👍 It's a draft at the moment because I want to try to write some integration tests today to make sure the retry logic works

@dirkmc dirkmc force-pushed the feat/mkts-miner-dont-dial-client branch 2 times, most recently from 6b713dc to e5ef95a Compare January 19, 2021 14:35
@dirkmc dirkmc force-pushed the feat/mkts-miner-dont-dial-client branch from e5ef95a to 1206154 Compare January 20, 2021 09:00
@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 20, 2021

Yesterday we performed a manual test.
The data transfer will restart correctly if the client or the miner restarts once during the transfer.
If the client or miner restarts twice during the transfer, the second time the data rate monitor in go-data-transfer may miss the restart because of ipfs/go-graphsync#141

However I think this is a big improvement, and there is also improved logging so we can see exactly what the client and miner side of the data transfer are doing. So I favour merging this PR and then bubbling up improvements to graphsync as they are released.

For future reference, to see all the relevant data transfer activity, the following log subsystems should be set at debug level:

  • markets
  • dt-pushchanmon
  • dt-impl
  • data_transfer_network

@magik6k magik6k merged commit 7a042ab into master Jan 20, 2021
@magik6k magik6k deleted the feat/mkts-miner-dont-dial-client branch January 20, 2021 12:37
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Feb 22, 2021
…kts-miner-dont-dial-client

Miner should not dial client on restart
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.

Miner should not try to dial the client to restart a data transfer
2 participants