-
Notifications
You must be signed in to change notification settings - Fork 0
feat: track block fetch bytes in flight #203
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
Conversation
089f855 to
d77b2a4
Compare
b318cd4 to
e5dbbd2
Compare
d77b2a4 to
0762f22
Compare
e5dbbd2 to
6c878c7
Compare
507e5e0 to
b38a4c8
Compare
6c878c7 to
105626a
Compare
b38a4c8 to
92e93e6
Compare
105626a to
4e82322
Compare
92e93e6 to
36a39d0
Compare
0e75bd2 to
aad0d3f
Compare
36a39d0 to
170066b
Compare
aad0d3f to
22d4eca
Compare
170066b to
152eefd
Compare
22d4eca to
77ee248
Compare
152eefd to
347de2d
Compare
77ee248 to
aa7e3a5
Compare
347de2d to
877af59
Compare
aa7e3a5 to
769eeea
Compare
780e2bf to
5bcdf6b
Compare
769eeea to
f2cf190
Compare
5bcdf6b to
ee4ce3f
Compare
9b69e71 to
b65be97
Compare
ee4ce3f to
d404350
Compare
|
🎉 All dependencies have been resolved ! |
b65be97 to
0759f6a
Compare
288ae0d to
c81ddaa
Compare
| Log.info "Waiting until ready to issue block fetch request..." | ||
| _ <- readMVar =<< gets (.stateVar) | ||
| Log.info "Ready to issue block fetch request!" | ||
| pure () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pure () |
| awaitMessage outChan = do | ||
| waitQSem handles.qSem | ||
|
|
||
| waitUntilReadyToFetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waiting like this would only make sense if this mini protocol was executed concurrently in multiple threads. else, who are we waiting for? this mini protocol is not executed concurrently in multiple threads, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid point. The current block fetch client is not pipelined (unlike ChainSync), so I think this flow control mechanism is never going to trigger.
@Gipphe either we pipeline the client or reevaluate whether this PR benefits us at all. I'm leaning toward the latter for now.
c81ddaa to
bd16a22
Compare
|
@Gipphe while reviewing I changed a few things to improve readability and opened a pull request targeting this branch. Please check and merge into this, then I'll approve it. |
bd16a22 to
fcb22a8
Compare
fcb22a8 to
272b66a
Compare
|
There is no pipelined version of the |
Track bytes in flight for current block fetch requests on a per-node basis and limit the bytes in flight accordingly. This strategy is adopted from the
cardano-node's own implementation of watermarks and in-flight byte tracking forBlockFetch.Depends on #200