Skip to content

Add service call timeout argument in spawner #1808

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.

Sign up for GitHub

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

Jump to bottom
Merged
christophfroehlich merged 10 commits into from
Nov 25, 2024
Merged

Add service call timeout argument in spawner #1808

christophfroehlich merged 10 commits into from
Nov 25, 2024

Conversation

@tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented Oct 22, 2024

This PR exposes the call_timeout of the spawner to be optionally set by the user

Angsa Deployment Team added 2 commits October 22, 2024 12:02
Add call timeout
63de32d 
Signed-off-by: Angsa Deployment Team <[email protected]>
fix cherrypick
26c79d3 
Signed-off-by: Angsa Deployment Team <[email protected]>
@tonynajjar tonynajjar mentioned this pull request Oct 22, 2024
Merged
@codecov
Copy link

codecov bot commented Oct 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.01%. Comparing base (23bd1c3) to head (a495547).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
+ Coverage   87.97%   88.01%   +0.03%     
==========================================
  Files         121      121              
  Lines       12403    12404       +1     
  Branches     1105     1105              
==========================================
+ Hits        10912    10917       +5     
+ Misses       1085     1083       -2     
+ Partials      406      404       -2
Flag Coverage Δ
unittests 88.01% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../controller_manager/controller_manager_services.py 79.85% <100.00%> (+0.74%) ⬆️
controller_manager/controller_manager/spawner.py 72.65% <100.00%> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

christophfroehlich
christophfroehlich requested changes Oct 25, 2024
View reviewed changes
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM, but please rebase/merge on master and update the release_notes and docs as I've done in this PR.

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Nov 6, 2024 •
edited
Loading

LGTM, but please rebase/merge on master and update the release_notes and docs as I've done in this PR.

Done. I believe CI is failing not because of this PR

christophfroehlich
christophfroehlich reviewed Nov 6, 2024
View reviewed changes
@christophfroehlich
Update controller_manager/doc/userdoc.rst
620e2ac 
Co-authored-by: Christoph Fröhlich <[email protected]>
christophfroehlich
christophfroehlich previously approved these changes Nov 6, 2024
View reviewed changes
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the follow-up!

@tonynajjar
fix order
a495547 
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar
Copy link
Contributor Author

tonynajjar commented Nov 22, 2024

I rebased to master and resolved conflicts caused by https://github.com/ros-controls/ros2_control/pull/1790/files. Could you please review @christophfroehlich @saikishor

saikishor
saikishor reviewed Nov 25, 2024
View reviewed changes
controller_manager/controller_manager/spawner.py Outdated
type=float,
)
parser.add_argument(
"--controller-manager-call-timeout",
Copy link
Member

@saikishor saikishor Nov 25, 2024

Choose a reason for hiding this comment

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

can we change this to --service-call--timeout? just to have a shorter naming? I'm saying as in future if we add more services, then it would be more generic.
@christophfroehlich what's your opinion on this one?

Copy link
Contributor Author

@tonynajjar tonynajjar Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I changed it already. It's an easy revert if @christophfroehlich disagrees. I don't have any strong opinion

@tonynajjar
rename
b537260 
Signed-off-by: Tony Najjar <[email protected]>
saikishor
saikishor previously approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

@saikishor
Update controller_manager/doc/userdoc.rst
22d383c 
Co-authored-by: Sai Kishor Kothakota <[email protected]>
christophfroehlich
christophfroehlich approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

@christophfroehlich christophfroehlich added the backport-humble Triggers PR backport to ROS 2 humble. label Nov 25, 2024
@christophfroehlich christophfroehlich merged commit 41d7393 into ros-controls:master Nov 25, 2024
19 of 20 checks passed
mergify bot pushed a commit that referenced this pull request Nov 25, 2024
@mergify
Add service call timeout argument in spawner (#1808)
0372e6d 
---------

Signed-off-by: Angsa Deployment Team <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Angsa Deployment Team <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
(cherry picked from commit 41d7393)

# Conflicts:
#	controller_manager/controller_manager/controller_manager_services.py
#	controller_manager/doc/userdoc.rst
#	doc/release_notes.rst
@mergify mergify bot mentioned this pull request Nov 25, 2024
Merged
christophfroehlich added a commit that referenced this pull request Nov 25, 2024
@christophfroehlich @saikishor
Add service call timeout argument in spawner (#1808)
efb323a 
---------

Signed-off-by: Angsa Deployment Team <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Angsa Deployment Team <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
christophfroehlich pushed a commit that referenced this pull request Dec 16, 2024
@mergify
Add service call timeout argument in spawner (#1808) (#1886)
1738050 
---------

Signed-off-by: Angsa Deployment Team <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Tony Najjar <[email protected]>
Co-authored-by: Angsa Deployment Team <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@saikishor saikishor saikishor approved these changes

@bijoua29 bijoua29 Awaiting requested review from bijoua29

@bmagyar bmagyar Awaiting requested review from bmagyar

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@livanov93 livanov93 Awaiting requested review from livanov93

@olivier-stasse olivier-stasse Awaiting requested review from olivier-stasse

Assignees

No one assigned

Labels

backport-humble Triggers PR backport to ROS 2 humble.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tonynajjar @christophfroehlich @saikishor