initial version of plugin based storage api #7

Karsten1987 · 2018-07-27T18:22:37Z

Connects to #8

This PR sets pluginlib in place to generically load storage plugins which implement this base class.

wjwwood

Do you expect the StorageInterface to have more functions in the future (e.g. seek, read or get_next, etc...)? Or will it be a different interface that operates on the StorageHandle?

Currently the StorageHandle class has no efficient way to compare itself to open bags, I imagine close must just string compare to find out, which might be ok for close, but if you need to do this to get the next message, then that's way too slow. Basically how are you going to do data association with the handle and the actual object that does the work inside of the StorageInterface (assuming that is what will operate on the open bags).

I'm also not sure about the naming, why is it rosbag2_storage::StorageHandle and not rosbag2_storage::Handle or rosbag2_storage::RosbagStorageHandle? The first alternative avoids any redundant information and the second one, while redundant, is identifiable without the namespace (StorageHandle without the namespace is pretty generic).

How will the user determine which format to use? Let's say the file path is file:///path/to/my_bag (no extension) and the factory can produce an interface for SQLite3 and ROS 1 bag. Is there some mechanism for storage backends to look at a file and say whether or not it's a valid one for that plugin (assuming the extension is wrong or missing)?

I was imagining that there would be a function like std::shared_ptr<StorageInterface> get_storage_interface_for_bag(std::string & bag_url), which would either return an instance of StorageInterface that works for that bag or raise an exception (or return an error code?) if none of the backends work with that file. How the implementations determine that is an implementation detail (doesn't matter), but the storage API or the user shouldn't have to figure out what backend if any should be used with a given file (otherwise it would need to know about all possible file type <-> plugin mappings ahead of time).

It should still be possible to try and force it to use a particular backend with a file (maybe for testing and corner cases), but the typical case should likely let the backends decide if a file can be opened or not.

wjwwood · 2018-07-30T23:51:06Z

rosbag2_storage/include/rosbag2_storage/storage_factory.hpp

+   */
+  ROSBAG2_STORAGE_PUBLIC
+  std::shared_ptr<StorageInterface>
+  get_storage_interface();


Shouldn't this function take a storage identifier? Otherwise it will never be able to return a specific implementation of the interface right? It's also documented in the doc block above.

The factory instance gets the identifier and thus get_storage_interface() shall only return one specific type of interface. But you're right, that might be misleading. We could maybe template the class with the interface type to have it a strongly typed factory?

Well at least I would update the documentation, since it says that this function takes the identifier. I guess a template argument is ok. I don't have the context to say if it's required/desired or not.

wjwwood · 2018-07-30T23:51:55Z

rosbag2_storage/include/rosbag2_storage/storage_handle.hpp

+typedef struct ROSBAG2_STORAGE_PUBLIC_TYPE StorageHandle
+{
+  std::string storage_identifier;
+  std::string file_path;


What is the file_path here? Might want to document this class and it's members.

wjwwood · 2018-07-30T23:52:37Z

rosbag2_storage/include/rosbag2_storage/storage_interface.hpp

+  /// Open the bag file given the specified path
+  /**
+   * The function opens the bag file and returns a handle to it.
+   * \param file_path the fqdn path to the bag to be loaded


fqdn doesn't make sense when talking about file paths... It stands for "Fully qualified domain name".

wjwwood · 2018-07-30T23:54:15Z

rosbag2_storage/include/rosbag2_storage/storage_interface.hpp

+  ROSBAG2_STORAGE_PUBLIC
+  virtual
+  void
+  set_storage_identifier(const std::string & storage_identifier) = 0;


Should this be a public method? Can you explain how you expect it to be used and by who (users versus implementers of the storage interface)?

auto instance = class_loader_->createSharedInstance(storage_identifier_); instance->set_storage_identifier(storage_identifier_); return instance;

Normally, I would set the storage_identifier directly at the constructor level. However, as every plugin gets loaded with pluginlib and thus requires a default constructor, I'd need a way of setting this identifier. I could put the identifier as a public member field, but I feel better about having this setter to avoid any external modification of this field.

Normally, I would set the storage_identifier directly at the constructor level. However, as every plugin gets loaded with pluginlib and thus requires a default constructor, I'd need a way of setting this identifier. I could put the identifier as a public member field, but I feel better about having this setter to avoid any external modification of this field.

Why is the storage identifier being put into the plugin? Shouldn't the plugin know what its identifier is?

wjwwood · 2018-07-30T23:57:55Z

rosbag2_storage/include/rosbag2_storage/storage_interface.hpp

+  ROSBAG2_STORAGE_PUBLIC
+  virtual
+  StorageHandle
+  open(const std::string & file_path) = 0;


This is probably fine for now, but in the future you might need to specify "bags" in a more complex way, e.g. it could be a folder, or a list of files, or a single file which contains multiple named "bags", or a URL to database (just imagining extremes), etc... Basically I'm not sure the assumption that a bag is uniquely identified by a single file path is going to scale. But it should be ok for now.

I'd say a parameter uri fits better since it can be used to describe any resource, not just files.

Karsten1987 · 2018-07-31T00:53:57Z

Thanks @wjwwood for the feedback. I'll address the comments ASAP.

You're right! This is early stage and not quite ready for review.
I just opened the PR for visibility. And to see how the pluginlib can be used for loading storage plugins.

Karsten1987 · 2018-07-31T18:37:29Z

I'm also not sure about the naming, why is it rosbag2_storage::StorageHandle and not rosbag2_storage::Handle or rosbag2_storage::RosbagStorageHandle?

Does that also apply then to rosbag2_storage::StorageFactory and rosbag2_storage::StorageInterface?

wjwwood · 2018-07-31T23:44:00Z

Yeah, I would personally choose rosbag2_storage::RosbagStorageFactory, though I might consider calling it rosbag2_storage::RosbagStorageInterfaceFactory, since it produces the interface you use to create "rosbag handles" (or just "rosbag"s?). In fact, I'm starting to question the purpose of the interface factory, though I don't have complete context.

Martin-Idel-SI · 2018-08-10T14:57:32Z

rosbag2_storage/include/rosbag2_storage/serialized_bag_message.hpp

+
+struct SerializedBagMessage
+{
+  rcutils_char_array_t serialized_data;


In principle, this is what I would've expected - except perhaps a shared_ptr<rcutils_char_array_t> because that seems to be the type we get in a subscription callback.

However, I have two questions:
a) The original design idea was to have librosbag2_storage independent of ros2. This part here would imply that we need at least a dependency on rcutils and/or rmw. Obviously, those are just type definitions, so it might be ok.
b) We tried to have a closer look and see how to write this to sqlite. It seems rcutils_char_array_t consists of four parts, the name-sake char array with the real content, two size bytes and another struct of allocator functions. I don't exactly understand the last part, the rcutils_allocator_t. We can't write it to a database (at least I wouldn't know how) and I don't understand how we can easily generate this part - can we always take the default from the rmw? How does this work in relation to part a) of my trouble?

a) The original design idea was to have librosbag2_storage independent of ros2. This part here would imply that we need at least a dependency on rcutils and/or rmw. Obviously, those are just type definitions, so it might be ok.

As much as possible it should be disconnected from ROS 2, to make it as portable as possible. If it's just rcutils then it shouldn't be too hard to do that, as rcutils doesn't really have any dependencies (no run dependencies, ament and python-empy as build depends). A dependency like rmw or rosidl, on the other hand, would be a red flag for me.

We tried to have a closer look and see how to write this to sqlite. It seems rcutils_char_array_t consists of four parts, the name-sake char array with the real content, two size bytes and another struct of allocator functions. I don't exactly understand the last part, the rcutils_allocator_t. We can't write it to a database (at least I wouldn't know how) and I don't understand how we can easily generate this part - can we always take the default from the rmw? How does this work in relation to part a) of my trouble?

You don't need to store the allocator structure in the database. That's just used to resize the serialized message and later to "delete it". Since the function that writes it to the database could essentially take a const rcutils_char_array_t *, the allocator would not be used nor is it needed when bringing it out of the database later.

On the other side (reading a bag), some allocator will be used to create the space in the outgoing rcutils_char_array_t instance so that the data from the database can be copied there.

There's no need for rmw in any of these steps. The allocator type and all related functions are also part of rcutils.

The other alternative would be to provide duplicate data types for the character array and allocator in this library itself. Converting between the version here and rcutils should be straightforward, but it would be nice to reuse what's in rctuils.

Thanks, sounds good to me. We'll go with this then!

Karsten1987 · 2018-08-13T20:55:11Z

Copy of the last comment from the feature branch #18 (comment)

We iterated a bit on the storage design. Basically, our suggestion would be:

We don't actually need a write-only-storage for now, so delete
Use exceptions instead of return booleans (this just changes the interface slightly)
Have a method open_readonly for read-only storage and open for general storage (i.e. the general storage interface has to implement both). This is better than only one "open" method for everything and the pluginlib has no problem with it
Since our initial demo was pushed to the branch, I pushed our changes for this, too - i.e. the demo would remain fully functional.

The interface does not yet use the RosbagSerializedMessage. This would be next.

…new one

…ing with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag

-In this way it is not confused with the storage id (e.g. sqlite3)

…ing with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag

-In this way it is not confused with the storage id (e.g. sqlite3)

* Display bag summary using `ros2 bag info` * Improve process execution helper to handle the working directory * Use metadata filename in sqlite storage to determine database name * GH-109 Write metadata file on Windows by hand - On Windows, the process is killed hard and thus does not write its metadata file - Since this is an issue with the test setup that seems very hard to fix, for now we just write the metadata file on our own * Remove empty bag folder if record gets aborted and no files are created - For example is neither --all nor topics are specified or if a non exsisting storage plugin is specified * Fail gracefully if a runtime error occurs when trying to record or play - For example if the storage plugin specified by the user at record does not exist * Log error in case of failing when loading metadata, and minor refactoring * Add comment to version field * Allow rosbag2 info without yaml file Currently only supported on rosbag2 side: - Allow passing a storage identifier to rosbag2::Info() - If a yaml file exists, read info from yaml - If no yaml file exists and a storage identifier was passed open storage and read info directly * GH-7 Don't try to read database name from metadata file when opening with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag * GH-7 Rename 'storage format' into 'serialization format' -In this way it is not confused with the storage id (e.g. sqlite3) * GH-7 Improve failure conditions * GH-7 Cleanup of superfluous forward declarations * GH-7 Further improve exception handling

* Updates the play_next API to play N messages. - Makes tests pass with the previous functionality (just one message). - No change to the ROS interface. * Changes PlayNext.srv interface definition and modifies the player accordingly. * Adds tests for play_next(N). * Update rosbag2_transport/src/rosbag2_transport/player.cpp * Apply suggestions from code review Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net> * Adds clarification string Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>

This was referenced Jul 30, 2018

Implement ros2 agnostic storage API #4

Closed

Implement ros2bag as part of new ros2cli verb #2

Closed

wjwwood reviewed Jul 31, 2018

View reviewed changes

Karsten1987 self-assigned this Jul 31, 2018

Karsten1987 added the in progress label Jul 31, 2018

Karsten1987 force-pushed the storage_interface branch from b2e20ac to 4da345a Compare August 9, 2018 20:59

Martin-Idel-SI reviewed Aug 10, 2018

View reviewed changes

Karsten1987 and others added 19 commits August 20, 2018 14:33

initial version of plugin based storage api

19171e4

rosbag2 serialized message

cc3b868

remove colcon ignores

1027a6e

Fix build and uncrustify

f584c2b

Add readable and writable storage interfaces

c198dc7

Delete first storage interface proposal and adapt storage factory to …

8df6de2

…new one

Modify test to work with new storage interfaces

2ae2a74

Prevent pluginlib from using boost

830e414

Introduce better logging using rcutils in rosbag_storage

984e93e

Add visibility control for Windows in rosbag_storage

53a900c

Rename visibility_control.h to visibility_control.hpp

5934ff0

Fix package.xml in rosbag2_storage

29f24b2

Cleanup CMakeLists in rosbag2_storage

c39ae4f

Adapt read() and write() methods signature

9990315

Use void * instead of char * in rosbag_storage

009fd4a

Add storage facade for plugins which are both readable and writable

1ea3a55

Extract bag_info struct to own file

18fa565

Change storage interface to have read/write access

1000d52

Add visibility to template specializations

956ba79

Martin-Idel-SI added 14 commits August 21, 2018 11:54

Delete superfluous classes + polishing

a64ca6a

Adapt SerializedBagMessage format

ad601bd

Let sqlite3 storage use new interface

9221fcc

Fix tests in rosbag2

f1e6500

Write and read only data

45ece72

Replace creation of shared instance by unmanaged instance

7051d51

Add pragma for windows

4d66842

Add visibility control for Windows

2fb1fda

Expose template definitions

bd37ba1

Move const to better location

e15912b

Replace strcpy

15c1d64

Delete superfluous methods

977eda7

Use visibility control in rosbag2

6e6ae9c

Minor cleanup

3dfc8f5

Karsten1987 added in review and removed in progress labels Aug 21, 2018

test for nullptr when opening storage

3892751

anhosi approved these changes Aug 21, 2018

View reviewed changes

Karsten1987 merged commit 05d24c8 into master Aug 21, 2018

Karsten1987 removed the in review label Aug 21, 2018

Karsten1987 deleted the storage_interface branch August 21, 2018 22:58

botteroa-si added a commit to bosch-io/rosbag2 that referenced this pull request Oct 24, 2018

ros2GH-7 Don't try to read database name from metadata file when open…

d7d35b8

…ing with ReadWrite io_flag - This avoids the logging of a 'failed to read metadata' error when recording a new bag

botteroa-si added a commit to bosch-io/rosbag2 that referenced this pull request Oct 25, 2018

ros2GH-7 Rename 'storage format' into 'serialization format'

9d18dd0

-In this way it is not confused with the storage id (e.g. sqlite3)

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Oct 29, 2018

ros2GH-7 Rename 'storage format' into 'serialization format'

9356463

-In this way it is not confused with the storage id (e.g. sqlite3)

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Oct 30, 2018

ros2GH-7 Improve failure conditions

7d26d6a

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Oct 30, 2018

ros2GH-7 Cleanup of superfluous forward declarations

e6dacb3

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Oct 30, 2018

ros2GH-7 Further improve exception handling

4e4b46a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial version of plugin based storage api #7

initial version of plugin based storage api #7

Karsten1987 commented Jul 27, 2018 •

edited

Loading

wjwwood left a comment

wjwwood Jul 30, 2018

Karsten1987 Jul 31, 2018

wjwwood Jul 31, 2018

wjwwood Jul 30, 2018

wjwwood Jul 30, 2018

wjwwood Jul 30, 2018

Karsten1987 Jul 31, 2018

wjwwood Jul 31, 2018

wjwwood Jul 30, 2018

greimela-si Aug 2, 2018

Karsten1987 commented Jul 31, 2018

Karsten1987 commented Jul 31, 2018

wjwwood commented Jul 31, 2018

Martin-Idel-SI Aug 10, 2018

wjwwood Aug 10, 2018

Martin-Idel-SI Aug 13, 2018

Karsten1987 commented Aug 13, 2018

initial version of plugin based storage api #7

initial version of plugin based storage api #7

Conversation

Karsten1987 commented Jul 27, 2018 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented Jul 31, 2018

Karsten1987 commented Jul 31, 2018

wjwwood commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 commented Aug 13, 2018

Karsten1987 commented Jul 27, 2018 •

edited

Loading