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

Track "default storage implementation" in a single place, to be used by other code #1116

Closed
emersonknapp opened this issue Oct 6, 2022 · 2 comments · Fixed by #1146
Closed
Assignees
Labels
enhancement New feature or request

Comments

@emersonknapp
Copy link
Collaborator

emersonknapp commented Oct 6, 2022

Description

Right now there are a variety of places in the rosbag2 core that encode the explicit string "sqlite3". This is a tight coupling between the core and the default storage implementation.

We should define in a single place what the default storage is, and use that wherever a default is required. Right now, this means finding all places where sqlite3 is mentioned explicitly, outside the sqlite3 storage implementation, and changing them so that it is not needed.

Related Issues

Completion Criteria

  • No explicit mention of "sqlite3" string anywhere in rosbag2 core
  • Allow empty storage id for record/write - Writer uses default storage implementation when not specified

Implementation Notes / Suggestions

Some options I can think of

  • Somehow use the ament_index to register a file from rosbag2_storage_default_plugins, which contains the name of the default plugin. This can be loaded by upstream packages
  • Provide an environment variable (via ament_environment_hooks) that user could override. ROSBAG2_DEFAULT_STORAGE maybe. If variable is unset for some reason, still want to have a builtin default to fall back to
  • Make rosbag2_storage_default_plugins provide a C++ header with a const string of the default plugin ID, which downstream packages can read and use. Expose this via rosbag2_py as well.

Testing Notes / Suggestions

  • All tests should run against the default storage implementation
@emersonknapp emersonknapp added the enhancement New feature or request label Oct 6, 2022
@emersonknapp emersonknapp self-assigned this Oct 6, 2022
@james-rms
Copy link
Contributor

james-rms commented Oct 21, 2022

@emersonknapp I might be missing something important, but the only places I can see sqlite3 being hardcoded in this repo are:

The rest seem to be part of test fixtures or performance evaluation code. Is there somewhere else I should be looking?

@james-rms
Copy link
Contributor

Plan:

  1. Expose a get_default_storage_id() function from rosbag2_storage, bind with pybind11
  2. Use this to set the default for ros2 bag record -s, and the default for Writer::open(std::string uri)
  3. for the tests, do a pass and split them between "sqlite3 specific" and "storage plugin agnostic". Leave the sqlite3-specific tests alone, and parametrize the storage plugin agnostic tests across all default-installed plugins (sqlite3, and soon MCAP).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants