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
feat(routing): add static routing table feature #3222
Conversation
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
Codecov Report
@@ Coverage Diff @@
## master #3222 +/- ##
==========================================
+ Coverage 90.33% 90.46% +0.12%
==========================================
Files 148 148
Lines 10297 10346 +49
==========================================
+ Hits 9302 9359 +57
+ Misses 995 987 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Did not think it through much, but doesn't it seem that we are using 2 arguments that are not compatible to activate only one feature? it seems we are using 2 bits to provide 1 bit of info?
if not self._static_routing_table: | ||
graph = RoutingTable(msg.envelope.routing_table) | ||
return graph.active_target_pod.expected_parts | ||
else: |
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.
Are we sure if static routing table, that num_part
will always be properly populated?
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 think so. At least for all non shared Pods (non external/regular) we know the information beforehand. For head peas it should be derived from needs
and for tail peas it should be derived from parallel
. As far as I can see it should be correct. But it might be worth adding more tests checking this, that is true
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.
Actually it did not work for the parallell
case because the pea args were not updated after the Flow had set the Pod args. I fixed this and added tests for this case.
I used two arguments, because on argument is a Flow global argument which defined 'this flow uses static routing tables' and the other args is defining per pod if it should send the routing table or not. At the moment the second argument is always automatically populated by the Flow depending on its configuration. |
I've added another optimization for grpc and static routing table: The target pods cant change so they are pre computed in init |
dd204e0
to
ba0b65a
Compare
This adds the new parameter
static_routing_table
to theFlow
. By default this parameter is disabled.If this parameter is enabled, the Flow will precompute the routing table for all Pods and set it via the args. If the routing table is statically set, all pods in this Flow will not send the routing table in the data requests and just use their own static routing table.
This does not work with
external
Pods as we cant set the routing table via args there.If we want to make this feature work with external Pods as well, we would need to implement some mechanism to distribute the routing table outside of data requests. That might be desirable soon, but I think at this point it is better and safer to do this rather minimal change, which will only do exactly what we need now.