Properly support restarting BatchedSend #5481

Activity

1. fjetter commented on Nov 2, 2021

   

   fjetter

   on Nov 2, 2021

   Member

   More actions

   If we want BatchedSend to be restartable, it should have a clear interface [...]

   How would that look like and what value would this deliver? So far, the restart by reconnect starts the BatchedComm again and the BatchedComm takes care of the rest. I'm wondering what value a "clear" interface would yield. It feels like every change to this interface would make the code using it more complex.

   [...] and tests for this.

   I personally consider the tests added in #5457 to be sufficient and if something is missing, let's discuss it over there.

   • Test for the worker disconnecting and restarting the batched comm. Ensure the payload is not lost and is properly submitted in the correct order after reconnect test_worker.py::test_dont_lose_payload_reconnect
   • The batched comm itself is ensured to retain any potentially lost payload if disconnected. upon reconnect the payload is properly submitted. test_batched.py:: test_retain_buffer_commclosed

   what is missing?
2. gjoseph92 commented on May 5, 2022

   

   gjoseph92

   on May 5, 2022

   Last edited by gjoseph92

   CollaboratorAuthor

   More actions

   Just for historical understanding—I believe this was sort of introduced in #3493. Before that, we were always creating a new BatchedSend instead of restarting the existing one:
   

   distributed/distributed/worker.py

   Lines 866 to 867 in 2a05299

   	self.batched_stream = BatchedSend(interval="2ms", loop=self.loop)
   	self.batched_stream.start(comm)

   Basic assert statements and documentation in BatchedSend.start would have made it clear to future developers that BatchedSend.start could not be called multiple times and prevented this bug. That's what I meant by a clear interface. Then that PR wouldn't have been able to so easily misuse a 5-year-old API and introduce this subtle error condition.
3. gjoseph92 commented on May 24, 2022

   

   gjoseph92

   on May 24, 2022

   CollaboratorAuthor

   More actions

   With worker reconnection removed, this is no longer necessary #6361. The API and internal validation should still be tightened up though: #6389.
4. gjoseph92

   closed this as completedon May 24, 2022
5. gjoseph92

   closed this as not plannedon May 24, 2022