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

Introducing Reindexer core #641

Merged
merged 51 commits into from
Mar 25, 2021
Merged

Conversation

jhdcs
Copy link
Contributor

@jhdcs jhdcs commented Feb 8, 2021

Replaces #539
Part of #395

After some discussions with the Tooling Working Group, I decided to do a major refactoring in order to implement all of their changes. This PR is going to be for the core reindexing functionality - it does not contain any Python API. That will come in a future PR, once this is polished up to an acceptable level.

This does not currently have the ability to reindex compressed bags, though I have added in test bags and target files for when such functionality becomes available.

@jhdcs jhdcs requested a review from a team as a code owner February 8, 2021 15:10
@jhdcs jhdcs requested review from emersonknapp and mjeronimo and removed request for a team February 8, 2021 15:10
@jhdcs jhdcs mentioned this pull request Feb 8, 2021
.gitignore Show resolved Hide resolved
rosbag2_cpp/include/rosbag2_cpp/reindexer.hpp Outdated Show resolved Hide resolved
rosbag2_cpp/include/rosbag2_cpp/reindexer.hpp Outdated Show resolved Hide resolved
rosbag2_cpp/include/rosbag2_cpp/reindexer.hpp Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/reindexer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/reindexer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/src/rosbag2_cpp/reindexer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_reindexer.cpp Outdated Show resolved Hide resolved
rosbag2_cpp/test/rosbag2_cpp/test_reindexer.cpp Outdated Show resolved Hide resolved
Jacob Hassold added 17 commits February 12, 2021 08:03
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Currently does not build.
Has a template error with making a unique
pointer around a StorageFactoryInterface
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Project builds again.
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Trying to figure out why we can't open multiple bags in a row...
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
and writing out the metadata file
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
@jhdcs jhdcs force-pushed the feature/reindexer_core branch from 23a0a0a to afb7c82 Compare February 12, 2021 13:04
Jacob Hassold added 6 commits February 12, 2021 09:21
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
@emersonknapp
Copy link
Collaborator

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

@emersonknapp
Copy link
Collaborator

I'm concerned about this OSX test - it seems like it failed both times. Do you have access to an OSX machine to try that out?

@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 1, 2021

I do not, unfortunately. And the error isn't very descriptive, is it?

@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 1, 2021

I just found this in the logs:

[ERROR] [1614384152.306481264] [rosbag2_storage]: Could not open '/Users/osrf/jenkins-agent/workspace/ci_osx/ws/src/jhdcs/rosbag2/rosbag2_cpp/test/rosbag2_cpp/reindex_test_bags/multiple_files/multiple_files_0.db3' with 'sqlite3'. Error: Failed to setup storage. Error: Error when processing SQL statement. SQLite error (14): unable to open database file
[ERROR] [1614384152.306521279] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.`

So it looks like it's trying to open the right file, but something with OSX doesn't like the format? Am I understanding that right?

Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 2, 2021

I found a location that looks like it should have been logging to the error stream, but was actually logging to the debug stream. It seems to be for the case that the storage factory is unable to find a registered class with the passed storage id (In this case, sqlite3). Since I'm not seeing any other errors in the OSX logs, I think this might be where the error occurs.

Would you be able to run the OSX tests one more time, and see if that error does show up? And if it does, what would cause sqlite3 to not be a registered class in OSX, but only for the reindex case? I don't see other tests failing when opening .db3 files, so it's got to be something special that OSX requires for the reindexer to do for registering classes...

@emersonknapp
Copy link
Collaborator

  • osx rebuild Build Status

Based on that analysis (thanks for continuing to dig!) - I noticed that rosbag2_storage_default_plugins isn't explicitly a test_depend of rosbag2_cpp (https://github.com/ros2/rosbag2/blob/master/rosbag2_cpp/package.xml). It is marked as exec_depend though, which I believe is incorrect (rosbag2_cpp should be completely independent of storage implementation).

Now that this has been brought to our attention - I am realizing that tests that depend on plugins being installed should be run through rosbag2_tests anyways - we don't want to build implementation dependencies into the core library APIs. Can you move this concrete reindexer test over to that package? Any more specific unit tests are still good to live in rosbag2_cpp

@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 2, 2021

Sure, I can do that. Glad to be of help!

I'm still going to leave that change from my last commit in, though. I think that probably was a bug.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Mar 2, 2021

I'm still going to leave that change from my last commit in, though. I think that probably was a bug.

That's fine - I believe I wrote that one as an error for compression plugins. Probably the plugin factories should get consolidated into a single bit of code at some point, it's duplicated right now.

Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 3, 2021

Moved the testing over. I'm noticing that the error "Requested storage id sqlite3 does not exist" just before I open each database in the bag. But the test passes, so I guess the system recovers.

Trying to track down WHY those messages pop up, and WHY it recovers, though...

@emersonknapp
Copy link
Collaborator

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

Jacob Hassold added 2 commits March 15, 2021 08:37
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
Distro A, OPSEC #4584

Signed-off-by: Jacob Hassold <[email protected]>
@emersonknapp
Copy link
Collaborator

OSX: Build Status

@emersonknapp
Copy link
Collaborator

emersonknapp commented Mar 25, 2021

Hey @jhdcs after testing it on the mackbook, I couldn't quite figure out what was wrong, but the database files in the multiple_files test were subtly misconfigured in some way that only showed up on OSX. I recreated the files with simple ros2 topic pub and ros2 bag record.

See #691 at fb8c4a5 for my tweaks. The only important part are the actual .db3 and the metadata file, feel free to just grab those directly and use them for the test (note there is only _0, _1, and _2 - I didn't see a specific need to have 4).

I rebased on master there as well, you're in fine shape, the only conflict is in rosbag2_tests/CMakeLists.txt - the if (fastrtps) if-check was removed so all those tests moved out an indentation level.

LMK how you want to proceed - you can resolve it from here, or I'm happy to push changes to this branch, run CI, and get it merged.

@jhdcs
Copy link
Contributor Author

jhdcs commented Mar 25, 2021

If you're willing to push the changes to the branch, that would be amazing! Please, go ahead! You are correct that the number of files in the bag doesn't really make a difference. I'm only trying to check that the reindexer tallies things properly when dealing with bag splitting. My only concern now is whether or not the other bagfiles I added are corrupt as well... I suppose we at least know what to look for, now.

What sort of corruption occurred? In order to create the original bagfile, I just used the talker/listener examples built in with ROS 2, run in a VM running Ubuntu 20.04. Perhaps sometime we should schedule a test where I create a bagfile, record ALL steps I took, and look for this corruption? As well as triple-check it can be run/reindexed on OSX... But that would be on a separate issue.

@emersonknapp
Copy link
Collaborator

What sort of corruption occurred?

I'm really not entirely sure. I was able to open the files with sqlite3 CLI on OSX, it was just the C++ API that was unable to open the files. It's worth investigating further if there is some issue of recording on Ubuntu Focal and playing back on OSX - but I don't want to block progress here on that.

I'll get this through to merge today.

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/64a7282df97ba795d6629acf27242136/raw/18f71a06ca711cf38f35718b361f917f8c1afc91/ros2.repos
BUILD args: --packages-up-to rosbag2_compression rosbag2_cpp rosbag2_storage rosbag2_storage_default_plugins rosbag2_tests
TEST args: --packages-select rosbag2_compression rosbag2_cpp rosbag2_storage rosbag2_storage_default_plugins rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7984

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

@emersonknapp emersonknapp merged commit ecc1097 into ros2:master Mar 25, 2021
@jhdcs jhdcs mentioned this pull request Mar 30, 2021
@jhdcs jhdcs deleted the feature/reindexer_core branch April 1, 2021 11:56
emersonknapp pushed a commit that referenced this pull request Aug 31, 2021
Add a new C++ Reindexer class for reconstructing metadata from bags that are missing it.

Distro A, OPSEC #4584
Signed-off-by: Jacob Hassold <[email protected]>
Signed-off-by: Emerson Knapp <[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.

2 participants