Don't retry by default when the stream is exhausted #91
Conversation
shsms
requested review from
llucax
and removed request for
phillip-wenig-frequenz
There was a problem hiding this comment.
LGTM. I think it makes sense in general, and will be useful when the microgrid API start streaming set power results (instead of using just one reply), but I wonder if you have any other use case where this is needed now.
Not approving because the CI is failing (and needs release notes), so I will probably need to approve again anyways.
When the stream is exhausted and the server closes the stream, we no longer automatically reconnect. But this can be changed by setting the `retry_on_exhausted_stream` parameter to `True`. Signed-off-by: Sahas Subramanian <[email protected]>
Based on how it is configured, the `GrpcStreamBroadcaster` can stop running when the retry limit has been reached or when the server closes the connection. The `is_running` method can be used to find out if that has happened. Signed-off-by: Sahas Subramanian <[email protected]>
This is because the default success case should never retry at all from now on. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
github-actions
bot
added
part:docs
The electricity trading API makes time limited streaming requests and at the end, the streamers need to exit, but they keep trying to reconnect and spam the logs. I've updated the tests and added release notes. |
There was a problem hiding this comment.
I was really confused by the parametrization with only one value. Given that the tests with retry_on_exhausted_stream with True and False are almost identical1, wouldn't it make more sense to just take the argument in the test too and check differently for each value?
Footnotes
-
Diff:
↩@@ -15,7 +15,10 @@ receiver_ready_event.set() async for item in receiver: items.append(item) - no_retry.next_interval.assert_called_once_with() + assert ( + no_retry.next_interval.call_count == 0 + ), "next_interval should not be called when streaming is successful" + assert items == [ "transformed_0", "transformed_1", @@ -26,8 +29,7 @@ assert caplog.record_tuples == [ ( "frequenz.client.base.streaming", - logging.ERROR, - "test_helper: connection ended, retry limit exceeded (mock progress), " - "giving up. Stream exhausted.", + logging.INFO, + "test_helper: connection closed, stream exhausted", ) ]
There was a problem hiding this comment.
I can only think of parameterising the cases as a way to make it look nice, but given that there are only two options, I'm not sure it is worth it.
2 participants
This change the default behaviour of
GrpcStreamBroadcasterto not reconnect when the server closes the stream. This can be overridden by setting theretry_on_exhausted_streamparameter toTrue.