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

Store serialized metadata in MCAP file #1423

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Store serialized metadata in MCAP file #1423

merged 3 commits into from
Sep 25, 2023

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Jul 15, 2023 •
edited
Loading

  • Store serialized metadata in MCAP file when calling update_metadata()
  • Read and parse serialized metadata from MCAP file when calling get_metada()
  • Remove ROS_DISTRO key-value from mcap::Metadata. ROS_DISTRO is now stored inside serialized metadata.

@MichaelOrlov MichaelOrlov marked this pull request as ready for review July 15, 2023 10:20
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner July 15, 2023 10:20
@MichaelOrlov MichaelOrlov requested review from emersonknapp, hidmic and james-rms and removed request for a team July 15, 2023 10:20
@MichaelOrlov MichaelOrlov changed the title Store serialized metadata in MCAP file when calling update_metadata() Store serialized metadata in MCAP file Jul 15, 2023
@MichaelOrlov MichaelOrlov mentioned this pull request Jul 18, 2023
Closed
@MichaelOrlov
Copy link
Contributor Author

@emersonknapp, @james-rms Friendly ping for review.

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/e9ccd97c296737b01bde125829ee7b33/raw/7db31f94c08adc2cf7d7daed19209af2153e3224/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12420

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

@MichaelOrlov
Copy link
Contributor Author

  • Re-run Windows CI after addressing test failure.
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor Author

Windows CI failure seems unrelated to the changes in this PR.
Failed PlayEndToEndTestFixture.play_filters_by_topic by timeout since was not able to receive confirmation about service call for player resume in 60 seconds.

  • Re-run Windows CI one more time
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor Author

The test failure reproduces on nightly builds as well according to the #1437

@MichaelOrlov MichaelOrlov mentioned this pull request Aug 22, 2023
Open
@MichaelOrlov
Store serialized metadata in MCAP file when calling update_metadata()
3ed23d0 
Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Remove ROS_DISTRO key-value from mcap::Metadata
a54fb93 
- ROS_DISTRO now stored inside serialized metadata

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Fix for test failure on Windows
cc6649a 
- Use `UnorderedElementsAreArray` for checking
`metadata.topics_with_message_count`.
- The list of the metadata.topics_with_message_count forming from the
 mcap_reader_->channels() which is unordered_map.
 The order of the elements in the unordered_map is unpredictable and
 can vary on different platforms.

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Sep 23, 2023 •
edited
Loading

I made some investigation in regards to failing test on Windows. And here is what I found:

  1. The failing test PlayEndToEndTestFixture.play_filters_by_topic is known to be flaky on Windows.
  2. The failure was reproducing on the baseline on the nightly jobs with the exact same error messages. See 🧑‍🌾 test_rosbag2_play_end_to_end flaky on Windows #1437 for details.
  3. The failure happening with the test parameter = 0
[ RUN      ] TestPlayEndToEnd/PlayEndToEndTestFixture.play_end_to_end_test/0
stdin is not a terminal or console device. Keyboard handling disabled.[INFO] [1691491248.586386600] [rosbag2_storage]: Opened database 'C:\ci\ws\src\ros2\rosbag2\rosbag2_tests\resources\sqlite3\cdr_test\cdr_test_0.db3' for READ_ONLY.

This corresponds to the SQLite3 blackened - therefore completely unrelated to changes in the mcap storage plugin.
4. The PlayEndToEndTests has already been marked as flaky for Windows.

I've rebased the current PR on top of the latest rolling and will rerun CI one more time - if CI will pass green I am going to merge this PR.

@MichaelOrlov
Copy link
Contributor Author

  • Re-run Windows CI after rebase
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor Author

The build failed due to the infrastructure issue

Summary: 14 packages finished [38.7s]
  1 package failed: fastcdr
  • Try to re-run Windows CI one more time
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/c16b69b352f571a641ff65ef193167d7/raw/71e06ad301a2b6c648fc7dadc12e0eb94356209a/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap
TEST args: --packages-above rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12690

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

@MichaelOrlov MichaelOrlov merged commit a029ef9 into rolling Sep 25, 2023
13 checks passed
@delete-merged-branch delete-merged-branch bot deleted the morlov/store_metadata_in_mcap branch September 25, 2023 04:42
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport iron

@mergify
Copy link

mergify bot commented Sep 26, 2023 •
edited
Loading

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 26, 2023
@MichaelOrlov @mergify
Store serialized metadata in MCAP file (#1423)
7ade61d 
* Store serialized metadata in MCAP file when calling update_metadata()

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

* Remove ROS_DISTRO key-value from mcap::Metadata

- ROS_DISTRO now stored inside serialized metadata

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

* Fix for test failure on Windows

- Use `UnorderedElementsAreArray` for checking
`metadata.topics_with_message_count`.
- The list of the metadata.topics_with_message_count forming from the
 mcap_reader_->channels() which is unordered_map.
 The order of the elements in the unordered_map is unpredictable and
 can vary on different platforms.

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

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit a029ef9)

# Conflicts:
#	rosbag2_storage_mcap/src/mcap_storage.cpp
@mergify mergify bot mentioned this pull request Sep 26, 2023
Closed
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport humble

@mergify
Copy link

mergify bot commented Sep 26, 2023 •
edited
Loading

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 26, 2023
@MichaelOrlov @mergify
Store serialized metadata in MCAP file (#1423)
360005e 
* Store serialized metadata in MCAP file when calling update_metadata()

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

* Remove ROS_DISTRO key-value from mcap::Metadata

- ROS_DISTRO now stored inside serialized metadata

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

* Fix for test failure on Windows

- Use `UnorderedElementsAreArray` for checking
`metadata.topics_with_message_count`.
- The list of the metadata.topics_with_message_count forming from the
 mcap_reader_->channels() which is unordered_map.
 The order of the elements in the unordered_map is unpredictable and
 can vary on different platforms.

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

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit a029ef9)

# Conflicts:
#	rosbag2_storage_mcap/src/mcap_storage.cpp
#	rosbag2_storage_mcap/test/rosbag2_storage_mcap/test_mcap_storage.cpp
@mergify mergify bot mentioned this pull request Sep 26, 2023
Closed
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-10-12/34178/1

MichaelOrlov added a commit that referenced this pull request Jun 13, 2024
@MichaelOrlov
Skip check for custom_metadata in test for mcap storage plugin.
9c73c2b 
- Rationale:
Mcap storage is not storing serialized metadata in a bag directly on
Humble. Backporting of the relevant #1423 is not possible without
updating mcap vendor package to the v1.1.0 because we have dependencies
from the mcap_reader_->metadataIndexes(); API which became available
only in the foxglove/mcap#902

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov mentioned this pull request Jun 13, 2024
Merged
MichaelOrlov added a commit that referenced this pull request Jun 13, 2024
@MichaelOrlov
Skip check for custom_metadata in test for mcap storage plugin.
0c0b038 
- Rationale:
Mcap storage doesn't store serialized metadata in bag directly on Iron
and Humble. Backporting of the relevant #1423 is not possible without
updating mcap vendor package to the v1.1.0 because we have dependencies
from the mcap_reader_->metadataIndexes(); API which became available
only in the foxglove/mcap#902

Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov added a commit that referenced this pull request Jun 23, 2024
@mergify @coalman321 @MichaelOrlov
[iron] Propagate "custom_data" and "ros_distro" in to the metadata.ya…
9e4b115 
…ml file during re-indexing (backport #1700) (#1711)

* Propagate "custom_data" and "ros_distro" in to the metadata.yaml file during re-indexing (#1700)

* propagate custom data and ros_distro during reindexing
Signed-off-by: Cole Tucker

Signed-off-by: coalman321 <[email protected]>

* fix ament uncrustify issue

Signed-off-by: coalman321 <[email protected]>

* only update metadata if size is nonzero

Signed-off-by: coalman321 <[email protected]>

* fix uncrustify issue and switch to empty

Signed-off-by: coalman321 <[email protected]>

* update reindexer test to verify custom data and ros_distro changes

Signed-off-by: coalman321 <[email protected]>

* Convert to use exact clock type for windows CI

Signed-off-by: coalman321 <[email protected]>

---------

Signed-off-by: coalman321 <[email protected]>
(cherry picked from commit 804432c)

* Remove "metadata.ros_distro" update since we don't have in on Iron

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

* Skip check for `custom_metadata` in test for mcap storage plugin.

- Rationale:
Mcap storage doesn't store serialized metadata in bag directly on Iron
and Humble. Backporting of the relevant #1423 is not possible without
updating mcap vendor package to the v1.1.0 because we have dependencies
from the mcap_reader_->metadataIndexes(); API which became available
only in the foxglove/mcap#902

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

---------

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

@emersonknapp emersonknapp emersonknapp approved these changes

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

@james-rms james-rms Awaiting requested review from james-rms

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

Successfully merging this pull request may close these issues.
3 participants
@MichaelOrlov @ros-discourse @emersonknapp