Arvados

Per discussion in IRC, the public interface of EventClient accidentally changed in #7658. For the sake of other clients, that interface needs to be restored. This means that EventClient needs to expose whatever public methods it had prior to #7658, including those inherited from WebSocketClient.

I think we should implement things that we previously inherited from WebSocketClient that are also implemented in PollClient. Presumably users are prepared to use either one, so it's less pressing to implement methods that wouldn't be available in PollClient anyway.

>>> import arvados.events as ae
>>> pc_methods = set(name for name in dir(ae.PollClient) if not name.startswith("_"))
>>> ws_methods = set(name for name in dir(ae.WebSocketClient) if not name.startswith("_"))
>>> ec_methods = set(name for name in dir(ae.EventClient) if not name.startswith("_"))
>>> missing_pc = pc_methods.difference(ec_methods)
>>> missing_ws = ws_methods.difference(ec_methods)
>>> missing_pc.intersection(missing_ws)
set(['run_forever', 'daemon', 'run'])

Upon reflection, I'm not sure my last comment is right. It feels unclear what the right thing to do is, because it's not clear what the API is "supposed" to be here.

The main way to get an events client is to call arvados.events.subscribe(). This will give you an EventClient (using Websockets) if it can and is allowed; or else it will give you a PollClient. If this is the way you're supposed to get an events client, then the API is the intersection of methods common to both classes. Those are subscribe, unsubscribe, run_forever, and close. (They both have a run method, too, but those methods have completely different signatures and semantics, so you can't safely call it on whatever subscribe() gives you back.)

But of course, there's nothing stopping clients from instantiating an EventClient directly--and if they want to ensure they get a Websockets connection, and don't fall back to polling, that's the only option available to them. If users do that, then we should err toward making all of the original EventClient methods available, including the ones inherited from WebSocketClient.

Either way, we have to add the run_forever method. That's easy enough. But it's less obvious to me whether the branch should make other methods available as well, or if it would actually be better to try to expose fewer methods in EventClient to enforce the idea that the proper interface is the small subset I listed above.

The intended API is that you use the module level subscribe() to get back an Event object, the public API of the event object is subscribe(), unsubscribe(), close(), and run_forever() (the last one should probably be renamed wait_for_close()). I would be fine with making everything else in module be private.

The intended behavior of run_forever() is to run until the socket is explicitly closed, however by delegating run_forever to the underlying Events object, it will only block until the current session terminates (despite the fact that we reconnect on unexpected close). Similarly, it doesn't make sense to delegate connect() and close_connection() for the same reason (no user of EventClient should be calling those methods).

Suggest that EventClient adopts the same approach as PollClient and uses an event variable to block until explicitly closed.

Peter Amstutz wrote:

Similarly, it doesn't make sense to delegate connect() and close_connection() for the same reason (no user of EventClient should be calling those methods).

Per note-8, I was just preserving the existing behavior: these methods are delegated in master. But it's done.

Suggest that EventClient adopts the same approach as PollClient and uses an event variable to block until explicitly closed.

Yes, thanks. Done in db2aa9f.

Thanks, LGTM @ db2aa9f

#9217 is the follow-up ticket to do the rest of the work to clean up the interfaces here.