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

[Humble] Add topics with zero message counts to the SQLiteStorage::get_metadata(). #1722

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Jun 22, 2024

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.

@MichaelOrlov i am trying to address for rolling with #1725.

can i borrow your code for testing? i am not sure if this works for rolling... any suggestions?

@MichaelOrlov MichaelOrlov marked this pull request as ready for review June 22, 2024 21:00
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner June 22, 2024 21:00
@MichaelOrlov MichaelOrlov requested review from gbiggs and hidmic and removed request for a team June 22, 2024 21:00
@MichaelOrlov
Copy link
Contributor Author

@fujitatomoya Sure, feel free to take any parts for rolling. The test should work for rolling as well; however, it will be better to parametrize it to run it for the mcap storage as well.

@MichaelOrlov MichaelOrlov force-pushed the morlov/add-topics-with-0-messages-to-metadata branch from cf510b9 to 3000974 Compare June 23, 2024 06:28
@MichaelOrlov
Copy link
Contributor Author

@fujitatomoya I've updated get_metadata_include_topics_with_zero_messages test to run it for mcap as well.
Feel free to take it for rolling. It should fit for rolling as is now. I assume that no modification needed.

@MichaelOrlov MichaelOrlov changed the title [Humble] Add topics with 0 messages to SQLiteStorage::get_metadata() [Humble] Add topics with zero message counts to the SQLiteStorage::get_metadata(). Jun 26, 2024
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 with green CI.

@MichaelOrlov thanks for the note. btw, this is also gonna go to iron after this humble fix, right? because #1725 only addressed rolling and jazzy.

@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Jul 22, 2024

@fujitatomoya Thanks for the review. We can propagate either this PR to the Iron or #1725. Need to double-check the base sources on Iron to see which one is better fit.

@MichaelOrlov
Copy link
Contributor Author

Pulls: #1722
Gist: https://gist.githubusercontent.com/MichaelOrlov/1a1a4f8ac0ad7670f8603b60e4eef957/raw/40ce70e63ea8ce5ccc15d405be9bd9194282d1d4/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_storage_default_plugins rosbag2_tests
TEST args: --packages-above rosbag2_cpp rosbag2_storage_default_plugins rosbag2_tests
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14288

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

@MichaelOrlov
Copy link
Contributor Author

The RHEL9 build fails with error message

--- stderr: rosbag2_storage_default_plugins
14:30:34 �[K/home/jenkins-agent/workspace/ci_linux-rhel/ws/src/ros2/rosbag2/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.c�[K In member functio�[Kvirtual rosbag2_storage::BagMetadata rosbag2_storage_plugins::SqliteStorage::get_metadat�[K’:
14:30:34 �[K/home/jenkins-agent/workspace/ci_linux-rhel/ws/src/ros2/rosbag2/rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp:539:��[Kerro��[Kfind�[K’ is not a member o�[K�[K’; did you mea�[Kf�[K’?
14:30:34   539 |       auto it = std::�[Kfind�[K(
14:30:34       |                      �[K^~~~�[K
14:30:34       |                      �[Kf�[K

@clalancette Do you have any idea what RHEL complaining about?

@MichaelOrlov MichaelOrlov force-pushed the morlov/add-topics-with-0-messages-to-metadata branch from 4bfbd95 to 0b1dd21 Compare July 25, 2024 02:44
@MichaelOrlov
Copy link
Contributor Author

Re-run RHEL CI job after attempt to fix a build error.

  • Linux-rhel Build Status

@MichaelOrlov
Copy link
Contributor Author

@clalancette I've fixed the RHEL build. I would appreciate if you would approve this backporting PR additionally so I can merge it.

@clalancette
Copy link
Contributor

@clalancette I've fixed the RHEL build. I would appreciate if you would approve this backporting PR additionally so I can merge it.

So unfortunately, that succeeding build on Humble is for RHEL-9, not RHEL-8 (which is what Humble supports). I think you should re-run the job on RHEL-8, and see what happens.

@MichaelOrlov
Copy link
Contributor Author

Run CI for RHEL8

  • Linux-rhel Build Status

- Parametrize get_metadata_include_topics_with_zero_messages test to run
for mcap and sqlite3 storage plugins.

Signed-off-by: Michael Orlov <[email protected]>
- Include <algorithm> header and explicitly use reference on
topic_metadata in a lambda capture list.

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov force-pushed the morlov/add-topics-with-0-messages-to-metadata branch from 0b1dd21 to 3f88538 Compare July 25, 2024 18:58
@MichaelOrlov
Copy link
Contributor Author

The warning message

Failed
rosbag2_py.flake8.E402 (./test/test_convert.py:30:1)

Failing for the past 1 build (Since [#1049](https://ci.ros2.org/job/ci_linux-rhel/1049/) )
Error Message
module level import not at top of file:
from rosbag2_py import (

is unrelated to the changes in this PR.
However, I've made a rebase and added suppression for those warning.

Re-run CI for RHEL8 after the rebase and suppressing PEP8:E402 warning

  • Linux-rhel Build Status

@MichaelOrlov
Copy link
Contributor Author

@clalancette Ok. Now the RHEL8 CI job is green. Ready to be approved.

@MichaelOrlov MichaelOrlov merged commit 5da1796 into humble Jul 26, 2024
14 checks passed
@MichaelOrlov MichaelOrlov deleted the morlov/add-topics-with-0-messages-to-metadata branch July 26, 2024 15:32
@fujitatomoya
Copy link
Contributor

either @clalancette or @MichaelOrlov can you create the backport PR by mergifyio? (i do not have permission on this repo)

@clalancette
Copy link
Contributor

either @clalancette or @MichaelOrlov can you create the backport PR by mergifyio? (i do not have permission on this repo)

I'm not sure what you mean; this one was the backport PR, no?

@fujitatomoya
Copy link
Contributor

@clalancette sorry for the complication,

i have consulted with @MichaelOrlov before, because of the base code difference, we did take the different pathes to rolling/jazzy and humble/iron. so that plan was to develop humble with this one and backport to iron which is missing now.

@MichaelOrlov MichaelOrlov mentioned this pull request Jul 27, 2024
@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Jul 27, 2024

@Mergifyio backport iron

Copy link

mergify bot commented Jul 27, 2024

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 27, 2024
…t_metadata(). (#1722)

(cherry picked from commit 5da1796)

# Conflicts:
#	rosbag2_py/test/test_convert.py
#	rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp
MichaelOrlov added a commit that referenced this pull request Aug 9, 2024
…t_metadata(). (#1722)

(cherry picked from commit 5da1796)

# Conflicts:
#	rosbag2_py/test/test_convert.py
#	rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp

Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov added a commit that referenced this pull request Aug 10, 2024
…metadata(). (backport #1722) (#1766)

* [Humble] Add topics with zero message counts to the SQLiteStorage::get_metadata(). (#1722)

(cherry picked from commit 5da1796)

# Conflicts:
#	rosbag2_py/test/test_convert.py
#	rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp

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

* Address merge conflicts

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

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
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.

Ros bag reindex failed to generate topic inside metadata.yaml if the message_count is 0
3 participants