-
Notifications
You must be signed in to change notification settings - Fork 209
add timeout arguments elsewhere #1185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rolling
Are you sure you want to change the base?
Changes from all commits
140c8fe
a08a582
c152967
1800294
658cba8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,10 @@ def add_arguments(self, parser, cli_name): # noqa: D102 | |
| parser.add_argument( | ||
| '--include-hidden-nodes', action='store_true', | ||
| help='Consider hidden nodes as well') | ||
| parser.add_argument( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to add a default timeout for
How about applying a consistent policy where verbs that may query multiple nodes default to 5.0s? |
||
| '--timeout', metavar='N', type=float, | ||
| help='Maximum time to wait for response in seconds ' | ||
| '(default: waits indefinitely)') | ||
|
|
||
| def main(self, *, args): # noqa: D102 | ||
| with NodeStrategy(args) as node: | ||
|
|
@@ -55,7 +59,8 @@ def main(self, *, args): # noqa: D102 | |
| with DirectNode(args) as node: | ||
| # Process and output each node's state immediately upon receipt | ||
| for query_node_name in nodes_to_query: | ||
| state = call_get_states(node=node, node_names=[query_node_name]) | ||
| state = call_get_states( | ||
| node=node, node_names=[query_node_name], timeout=args.timeout) | ||
| state = state.get(query_node_name) | ||
|
|
||
| if isinstance(state, Exception): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to include the node name in timeout error messages for
call_get_states(and related functions)?In this PR, most timeout errors already include the node or service name, but the lifecycle functions (
call_get_states,call_change_states) currently do not. This can be confusing for users ofcall_get_states, sinceros2 lifecycle getmay query multiple nodes in a loop, making it hard to identify which node was unresponsive when a timeout occurs.