Skip to content

Comments

Add cli option compression-threads-priority#1768

Merged
MichaelOrlov merged 5 commits intoros2:rollingfrom
r7vme:1760-add-cli
Aug 9, 2024
Merged

Add cli option compression-threads-priority#1768
MichaelOrlov merged 5 commits intoros2:rollingfrom
r7vme:1760-add-cli

Conversation

@r7vme
Copy link
Contributor

@r7vme r7vme commented Jul 31, 2024

This PR adds --compression-threads-priority CLI parameter for the ros2 bag record command to be able to set up compression threads priority.

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
@r7vme r7vme marked this pull request as ready for review July 31, 2024 12:08
@r7vme r7vme requested a review from a team as a code owner July 31, 2024 12:08
@r7vme r7vme requested review from MichaelOrlov and hidmic and removed request for a team July 31, 2024 12:08
@r7vme
Copy link
Contributor Author

r7vme commented Jul 31, 2024

@MichaelOrlov PTAL

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
@r7vme
Copy link
Contributor Author

r7vme commented Aug 5, 2024

@fujitatomoya thanks for the review. I see a lot of unrealed issues in CI

ImportError: cannot import name 'Unpack' from 'typing_extensions' 

Did you see smth similar?

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
- Rationale: To test the same behavior as in the writer factory class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@r7vme Thanks for your contribution.
Overall looks good. However, I added some minor changes in the help section and doxygen comments to better explain what values could be for the compression thread's priority.

@MichaelOrlov
Copy link
Contributor

Pulls: #1768
Gist: https://gist.githubusercontent.com/MichaelOrlov/40e5af023be832e0841810afeb3dba63/raw/b156ba663ce90a1cdb900f95d4c4641629d3816d/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression ros2bag rosbag2_transport rosbag2_py
TEST args: --packages-above rosbag2_compression ros2bag rosbag2_transport rosbag2_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14391

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@r7vme
Copy link
Contributor Author

r7vme commented Aug 9, 2024

@MichaelOrlov LGTM, thanks

@MichaelOrlov
Copy link
Contributor

Linux CI job failed due to insufficient disk space on the test machine.
Re-running CI job one more time after clean up space.

  • Linux Build Status

@MichaelOrlov
Copy link
Contributor

Re-running CI job one more time after one more attempt to fix build infrastructure.

  • Linux Build Status

@MichaelOrlov MichaelOrlov merged commit 25c3e1c into ros2:rolling Aug 9, 2024
@MichaelOrlov
Copy link
Contributor

https://github.com/Mergifyio backport jazzy

@mergify
Copy link

mergify bot commented Aug 9, 2024

backport jazzy

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Aug 9, 2024
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Fix CI issues

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Use integer type for compression threads priority default value in tests

- Rationale: To test the same behavior as in the writer factory class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 25c3e1c)
MichaelOrlov pushed a commit that referenced this pull request Aug 9, 2024
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Fix CI issues

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Use integer type for compression threads priority default value in tests

- Rationale: To test the same behavior as in the writer factory class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 25c3e1c)
MichaelOrlov pushed a commit that referenced this pull request Aug 10, 2024
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Fix CI issues

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Use integer type for compression threads priority default value in tests

- Rationale: To test the same behavior as in the writer factory class

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 25c3e1c)

Co-authored-by: Roman <rsokolkov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CLI option to set compression threads priority for Rosbag2 recorder

4 participants