feat: congestion control for catchunks#132
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.
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. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| type CongestionWindow interface { | ||
| String() string | ||
|
|
||
| EventChannel() <-chan WindowEvent // where window events are emitted |
There was a problem hiding this comment.
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.
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.
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.
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.
This interface has not been implemented I guess?
There was a problem hiding this comment.
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 |
zjkmxy
left a comment
There was a problem hiding this comment.
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.
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.
[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()) |
There was a problem hiding this comment.
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
TryStoreas in ac07ccaArgument parsing for congestion window parameters(need to changeClientAPIs)