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

Handle data-sent and data-queued events in the TransferFinished state #233

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

aarshkshah1992
Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 commented Aug 18, 2021

Closes #234 .

  • After a data-transfer restart, Graphsync can still call the OnDataSent hook for blocks that have already been sent which in turn will fire the DataSent data-transfer event. The same applies to the DataQueued event.
  • To ensure we don't double count data sent, we introduced a DataSentProgress and a DataQueuedProgress events a while ago in Dont double count data sent #185. These events are only emitted the first time we send a block to a remote peer.
  • Thus, if a data-transfer restarts after entering the TransferFinished state, we can still receive DataSent and DataQueued events for blocks that have already been sent. We need to handle these events gracefully in the TransferFinished state. We do see a LOT of restarts in Estaury's logs for data-transfer across the board. See attached.
  • Note however, that we should NOT see the DataSentProgress and a DataQueuedProgress events in the TransferFinished state and this is confirmed by Estuary's logs.

Note: I don't think this error actually causes storage deals to fail but still nice to fix to clean up the logs.

shuttle-logs-bad-tx3 (1).gz

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

CI failing due to gofmt issues.

Comment on lines 165 to 166


Copy link
Member

Choose a reason for hiding this comment

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

needs gofmt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies, need to setup gofmt on my new Goland installation.

Comment on lines 68 to 70
chst.AddLog("")
return nil
}),
Copy link
Member

Choose a reason for hiding this comment

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

Bad indenting here and below.

@codecov-commenter
Copy link

Codecov Report

Merging #233 (b6e8d89) into master (0e3b2a1) will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   67.44%   67.70%   +0.26%     
==========================================
  Files          24       24              
  Lines        3044     3044              
==========================================
+ Hits         2053     2061       +8     
+ Misses        636      627       -9     
- Partials      355      356       +1     
Impacted Files Coverage Δ
channels/channels_fsm.go 70.54% <100.00%> (+5.47%) ⬆️
network/libp2p_impl.go 68.38% <0.00%> (-2.95%) ⬇️
channels/channels.go 74.64% <0.00%> (+1.91%) ⬆️

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 0e3b2a1...b6e8d89. Read the comment docs.

@aarshkshah1992 aarshkshah1992 merged commit 8931e01 into master Aug 18, 2021
@aarshkshah1992 aarshkshah1992 deleted the fix/state-transitions branch August 18, 2021 11:19
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.

DataSent and DataQueued events should be handled gracefully in the TransferFinished state
3 participants