Arvados

Comments @ 7b2cd50

Could you refactor the code for managing the list of "filters" so that it is the same between EventClient and PollClient? Maybe add a small helper class.

_EventClient.__init__() the on_closed parameter should not be optional.

In EventClient.on_closed() it should catch exceptions around self.ec.connect() and keep retrying self.ec.connect() with a 5-10 second wait in between tries until it succeeds or self.is_closed == True. I didn't think of this detail when I wrote the original description but it is important for robustness (for example, when the websockets service reboots and is unavailable for a minute or two). The retry logic will need a test.

The test isn't quite correct. It creates human2 before calling close_connection(), so it is possible for it to receive the event before disconnecting. It should create human2 after it calls close_connection() to demonstrate that it reconnected after close_connection().

• Could you refactor the code for managing the list of "filters" so that it is the same between EventClient and PollClient? Maybe add a small helper class.

The PollClient is using [filters] where as EventClient is using filters as is which as an array of array, so I did not do this. However, I noticed an issue with the way I was managing "subscriptions" and re-subscribing on unexpected closes. I updated the logic to first connect (with the original filters) and then issue submit calls for each entry found in the unsubscribed subscriptions.

• EventClient._init__() the on_closed parameter should not be optional.

Done

• In EventClient.on_closed() it should catch exceptions around self.ec.connect() and keep retrying self.ec.connect() with a 5-10 second wait in between tries until it succeeds or self.is_closed == True.

Added this code update

• The retry logic will need a test.

In order to write a test that checks the retry logic, I would need to mock the EventClient.connect with side_effects array [pass, some_error, pass], where the first and third side_effects should pass and do the actual connect work. I could not figure out how to define side_effects such that they pass the first and third time onwards but fail the second time. Please provide a working code sample for this so that I can write this test. Thanks.

• The test isn't quite correct. It creates human2 before calling close_connection(), so it is possible for it to receive the event before disconnecting. It should create human2 after it calls close_connection() to demonstrate that it reconnected after close_connection().

Done

Added test for retry during reconnect.

Suggest using StreamIO.StreamIO instead of logging.FileHandler. This buffers in memory instead of creating a temporary file:

        logstream = StreamIO.StreamIO()
        rootLogger = logging.getLogger()
        streamHandler = logging.StreamHandler(logstream)
        rootLogger.addHandler(streamHandler)

        ...
        log_messages = logstream.getvalue()

In EventClient.on_closed, this should catch Exception and log the exception:

              except Exception as e:
                  _logger.warn("Error '%s' during websocket reconnect. Will retry after 5s.", e)
                  time.sleep(5)