control packet support in test::Proxy #802
Conversation
|
🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats. |
github-actions
bot
added
the
contribution
| Proxy(const roc_endpoint* receiver_source_endp, | ||
| const roc_endpoint* receiver_repair_endp, | ||
| const roc_endpoint* receiver_control_endp, | ||
| size_t n_source_packets, | ||
| size_t n_repair_packets, | ||
| size_t n_control_packets, | ||
| unsigned flags) | ||
| : packet_pool_("proxy_packet_pool", arena_) | ||
| , buffer_pool_("proxy_buffer_pool", arena_, 2000) | ||
| , queue_(packet::ConcurrentQueue::Blocking) | ||
| , net_loop_(packet_pool_, buffer_pool_, arena_) |
There was a problem hiding this comment.
It's not a good idea to copy-paste this huge constructor. If we need two constructors, we'd need to extract common code into a method and reuse it.
However, in this case I think it's better to have just one constructor and pass NULL for control endpoint in tests that don't use it. It's better to make it explicit.
I saw your message about backwards compatibility but didn't have a chance to answer before you opened PR, sorry. We don't need to preserve compatibility here and want to update tests instead.
| const roc_endpoint* control_endpoint() const { | ||
| return input_control_endp_; | ||
| } |
There was a problem hiding this comment.
| const roc_endpoint* control_endpoint() const { | |
| return input_control_endp_; | |
| } | |
| const roc_endpoint* control_endpoint() const { | |
| CHECK(input_control_endp_); | |
| return input_control_endp_; | |
| } |
Let's make it fail early if trying to use control endpoint when it's not configured.
| const roc_endpoint* receiver_control_endp, | ||
| size_t n_source_packets, | ||
| size_t n_repair_packets, | ||
| size_t n_control_packets, |
There was a problem hiding this comment.
n_control_packets is not a thing. source and repair packets are grouped into a FEC block, but control packets are not part of any block. We can just remove this parameter.
| const size_t block_pos = pos_ % (n_source_packets_ + n_repair_packets_); | ||
| const size_t block_pos = | ||
| pos_ % (n_source_packets_ + n_repair_packets_ + n_control_packets_); |
There was a problem hiding this comment.
As mentioned above, n_control_packets_ doesn't make sense, let's remove it. We can just never drop control packets for now, if we'll ever need it, we can add separate flag for that.
| if (pp->udp()->dst_addr == recv_source_config_.bind_address) { | ||
| pp->udp()->dst_addr = receiver_source_endp_; | ||
| LONGS_EQUAL(status::StatusOK, source_queue_.write(pp)); | ||
| } else if (pp->udp()->dst_addr == recv_control_config_.bind_address) { | ||
| pp->udp()->dst_addr = receiver_control_endp_; | ||
| LONGS_EQUAL(status::StatusOK, control_queue_.write(pp)); | ||
| } else { | ||
| pp->udp()->dst_addr = receiver_repair_endp_; | ||
| LONGS_EQUAL(status::StatusOK, repair_queue_.write(pp)); | ||
| } |
There was a problem hiding this comment.
nitpick: let's reorder branches in a familiar way: source, repair, control.
|
The big part missing from the PR is bi-directional proxying of the control packets - control packets are sent in two directions, unlike source and repair packets. Actually that's the most important part of this feature (the rest is more straightforward), please read issue text in #772. |
gavv
added
status: needs revision
#772
Summary
This PR adds support for control packet handling in
Proxywhile maintaining backward compatibility with existing tests.Changes Implemented
Proxyclass to handle control packets in addition to source and repair packets.write()to use control queue integrated into its current use of source/repair queues to forward control packets.Development Workflow Compliance
developbranch, per project guidelines.scons -Q fmt) before submission.Notes
Proxy, following the same queuing mechanism as source and repair packets.