Skip to content
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

actions/get_result has a different default timeout. #372

Merged
merged 5 commits into from
Jan 11, 2025

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Jan 2, 2025

As described #369, it would be better to have a different default timeout for get_result service in ROS 2 action.

Here are some questions:

  1. I set the timeout to 300 seconds, but is that enough for most user cases?
  2. Should we separate the actions/get_result from the queries_timeout in the configuration file?

Copy link

github-actions bot commented Jan 2, 2025

PR missing one of the required labels: {'internal', 'breaking-change', 'dependencies', 'new feature', 'enhancement', 'bug', 'documentation'}

@evshary evshary added the invalid This doesn't seem right label Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

PR missing one of the required labels: {'bug', 'dependencies', 'new feature', 'breaking-change', 'internal', 'enhancement', 'documentation'}

@evshary evshary added bug Something isn't working and removed invalid This doesn't seem right labels Jan 2, 2025
@evshary evshary requested a review from JEnoch January 7, 2025 08:21
Copy link
Member

@JEnoch JEnoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made 2 minor changes to your PR:

  • clarification in DEFAULT_CONFIG.json5 description
  • make that the default config is reflected in the log, instead of being displayed as None. Thus the configured timeout is more clear.

Please review also on your side and merge if you approve.

@evshary
Copy link
Contributor Author

evshary commented Jan 11, 2025

I made 2 minor changes to your PR:

  • clarification in DEFAULT_CONFIG.json5 description
  • make that the default config is reflected in the log, instead of being displayed as None. Thus the configured timeout is more clear.

Please review also on your side and merge if you approve.

Much better than the original one!
LGTM, but I don't have permission to merge it.

@JEnoch JEnoch merged commit fd977fb into eclipse-zenoh:main Jan 11, 2025
9 checks passed
@evshary evshary deleted the config_for_action branch January 11, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants