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.

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

Add cli option compression-threads-priority #1768

Merged
merged 5 commits into from
Aug 9, 2024
Merged

Add cli option compression-threads-priority #1768

merged 5 commits into from
Aug 9, 2024

Conversation

r7vme
Copy link
Contributor

@r7vme r7vme commented Jul 31, 2024 •
edited by MichaelOrlov
Loading

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

@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

fujitatomoya
fujitatomoya approved these changes Aug 2, 2024
View reviewed changes
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

ros2bag/test/test_record_with_compression_thread_priority.py Outdated Show resolved Hide resolved
@r7vme
Fix CI issues
eacc1f2 
Signed-off-by: Roman Sokolkov <[email protected]>
@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?

MichaelOrlov
MichaelOrlov approved these changes Aug 9, 2024
View reviewed changes
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
13 of 14 checks passed
@MichaelOrlov
Copy link
Contributor

https://github.com/Mergifyio backport jazzy

@mergify Mergify
Copy link

mergify bot commented Aug 9, 2024 •
edited
Loading

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 9, 2024
@r7vme @mergify
Add cli option compression-threads-priority (#1768)
b880ffb 
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <[email protected]>

* Fix CI issues

Signed-off-by: Roman Sokolkov <[email protected]>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <[email protected]>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Roman Sokolkov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
(cherry picked from commit 25c3e1c)
@mergify mergify bot mentioned this pull request Aug 9, 2024
Merged
MichaelOrlov pushed a commit that referenced this pull request Aug 9, 2024
@r7vme @MichaelOrlov
Add cli option compression-threads-priority (#1768)
72ea3bb 
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <[email protected]>

* Fix CI issues

Signed-off-by: Roman Sokolkov <[email protected]>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <[email protected]>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Roman Sokolkov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
(cherry picked from commit 25c3e1c)
MichaelOrlov pushed a commit that referenced this pull request Aug 10, 2024
@mergify @r7vme
Add cli option compression-threads-priority (#1768) (#1778)
1a5ba7a 
* Add cli option compression-threads-priority

Signed-off-by: Roman Sokolkov <[email protected]>

* Fix CI issues

Signed-off-by: Roman Sokolkov <[email protected]>

* Add timeout for the test_priority_propagated_into_compression_thread

Signed-off-by: Michael Orlov <[email protected]>

* Update help section and doxygen comments for thread priority parameters

Signed-off-by: Michael Orlov <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Roman Sokolkov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
(cherry picked from commit 25c3e1c)

Co-authored-by: Roman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner automatically assigned from ros2/tooling-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CLI option to set compression threads priority for Rosbag2 recorder
3 participants
@r7vme @MichaelOrlov @fujitatomoya