-
Notifications
You must be signed in to change notification settings - Fork 18
feat: congestion control for catchunks #132
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
bc237b7
to
48afe7c
Compare
Introduce "std/object/congestion" subpackage providing a unified interface for windowed congestion control and three window strategies: fixed, AIMD, and CUBIC Introduce a basic interface for RTT estimators
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.
Pull Request Overview
This PR introduces a congestion control mechanism for catchunks by adding multiple congestion window strategies and integrating a retransmission queue. Key changes include:
- Adding a new RTTEstimator interface for RTT measurement.
- Implementing three congestion window strategies: Fixed, AIMD, and CUBIC.
- Refactoring the client consumer to leverage the new congestion control mechanism with retransmission queue support.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
std/object/congestion/rtt_estimator.go | Adds the RTTEstimator interface for RTT estimation. |
std/object/congestion/congestion_window_fixed.go | Introduces a fixed congestion window implementation. |
std/object/congestion/congestion_window_cubic.go | Implements the CUBIC congestion window strategy with updates. |
std/object/congestion/congestion_window_aimd.go | Implements the AIMD congestion control mechanism. |
std/object/congestion/congestion_window.go | Provides the generic CongestionWindow interface. |
std/object/client_consume_seg.go | Refactors client consumption logic to use congestion control. |
type CongestionWindow interface { | ||
String() string | ||
|
||
EventChannel() <-chan WindowEvent // where window events are emitted |
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.
Is it used anywhere?
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.
Is it used anywhere?
It's not used for now. I put the code there so that it is easier to build custom listeners.
By default (no channel specified) the event is passed to the logger by log.Debug
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.
Then, in your code, this channel will always block and the window outputs every event to the log.
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.
Yes, since we don't have any listeners at the moment. I feel it is reasonable to output to the log in this case so that we don't block the whole thing.
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.
I don't think there would be Listeners at all.... Let's just put them to logging.
) | ||
|
||
// RTTEstimator provides an interface for estimating round-trip time. | ||
type RTTEstimator interface { |
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 interface has not been implemented I guess?
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 interface has not been implemented I guess?
Yeah it hasn't... I remembered thinking if a TCP-like RTT estimator is enough due to the presence of cachings and multipath, etc. The CUBIC implementation will not enter the TCP-friendliness mode if an RTT estimator is not present.
Oh I missed the congestion module |
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.
LGTM
I'm not familiar with the algorithms and it seems the RTT estimator is not done. Merge to unblock.
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.
Pull Request Overview
This PR introduces a congestion control mechanism for catchunks by adding an interface for multiple congestion window strategies and refactoring the client segment fetching logic to support retransmission.
- Added new interfaces for RTT estimation and congestion window strategies (Fixed, AIMD, CUBIC).
- Refactored client consumption state and segment fetching to use a structured fetching window and a retransmission queue.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
std/object/congestion/rtt_estimator.go | New interface for round-trip time estimation. |
std/object/congestion/congestion_window_fixed.go | Implements a fixed window congestion control strategy. |
std/object/congestion/congestion_window_cubic.go | Implements CUBIC congestion control with custom update logic. |
std/object/congestion/congestion_window_aimd.go | Implements AIMD congestion control strategy. |
std/object/congestion/congestion_window.go | Defines the common CongestionWindow interface and associated signals. |
std/object/client_consume_state.go | Refactors window tracking from raw indices to a FetchWindow structure. |
std/object/client_consume_seg.go | Updates segment fetching logic with a retransmission queue and window-based flow control. |
std/object/client_consume.go | Uses the new FetchWindow structure for client consumption state. |
log.Debug(nil, "Checking for work") | ||
|
||
// check if the window is full | ||
if s.IsCongested() { | ||
log.Debug(nil, "Window full", "size", s.window.Size()) |
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.
[nitpick] Consider passing a non-nil context or identifier to log.Debug for improved traceability instead of using nil.
log.Debug(nil, "Checking for work") | |
// check if the window is full | |
if s.IsCongested() { | |
log.Debug(nil, "Window full", "size", s.window.Size()) | |
log.Debug(s.id, "Checking for work") | |
// check if the window is full | |
if s.IsCongested() { | |
log.Debug(s.id, "Window full", "size", s.window.Size()) |
Copilot uses AI. Check for mistakes.
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.
Valid comment but I don't think it's that important.
Related to #97
Summary
This PR introduces congestion control mechanism for catchunks
Improvements
Pending Changes/Additions
TryStore
as in ac07ccaArgument parsing for congestion window parameters(need to changeClient
APIs)