[RSDK-2485] Client Reconnect #260
Conversation
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
njooma
force-pushed
the
RSDK-2485/client-reconnect
branch
from
dbf458b to
d309d83
Compare
src/viam/robot/client.py
Outdated
| if rname in self._manager.resources: | ||
| res = self._manager.get_resource(ResourceBase, rname) | ||
| if isinstance(res, ReconfigurableResourceRPCClientBase): | ||
| res.reset_channel(self._channel) | ||
| else: | ||
| self._manager.remove_resource(rname) | ||
| self._manager.register( | ||
| Registry.lookup_subtype(Subtype.from_resource_name(rname)).create_rpc_client(rname.name, self._channel) | ||
| ) | ||
| else: | ||
| try: | ||
| self._manager.register( | ||
| Registry.lookup_subtype(Subtype.from_resource_name(rname)).create_rpc_client(rname.name, self._channel) | ||
| ) | ||
| except ResourceNotFoundError: | ||
| pass |
There was a problem hiding this comment.
[nit] it feels like this could factored out into a helper method - _create_or_reset_client or something like that?
src/viam/module/module.py
Outdated
| """Start the module service and gRPC server""" | ||
| try: | ||
| await self.server.serve(log_level=self._log_level, path=self._address) | ||
| except SystemExit as e: |
There was a problem hiding this comment.
what does this do and why do we need it? maybe leave a comment around this bit
There was a problem hiding this comment.
cool, thanks. @benjirewis does this solve https://viam.atlassian.net/browse/RSDK-2551?
There was a problem hiding this comment.
It does not resolve RSDK-2551, and I'm actually not sure we should add this @njooma (totally open to other opinions). The except here doesn't actually catch anything from grpclib. It's confusing, but the only error you can catch from grpclib's second stage of graceful exit is asyncio.CancelledError. From what I can tell, there's no way to catch the raised SystemExit(143).
njooma
force-pushed
the
RSDK-2485/client-reconnect
branch
from
42f4d29 to
e2286c0
Compare
|
Removing myself from reviewers since I have insufficient background on this codebase to be super productive; happy to test this on prior failure modes ones it's merged though |
Yup, just put you on so you could track progress |
| with self._lock: | ||
| return [r for r in self._resource_names] | ||
|
|
||
| def _close_channel(self, *, tab_count=0): |
There was a problem hiding this comment.
(q) I'm not sure I get why the tab_count exists. Why do we want to indent when calling close() specifically?
There was a problem hiding this comment.
Because I call this internal function from a 2 difference places, and I want the logs to reflect a certain level of indentation depending on where it was called from (e.g. if it was called as part of a task that closes a lot of things, it should be indented. if it was closed on its own, it should not be indented)
This PR tackles a few issues:
ReconfigurableResourceRPCClientBasetype which all built in resource clients useFor reviewers, the main file you should be checking is
robot.client