-
Notifications
You must be signed in to change notification settings - Fork 242
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
Rosbag splitting in Writer #185
Rosbag splitting in Writer #185
Conversation
Epr failed due to a slight change in db name - this PR removed the extension since I can resolve this by either:
I'll probably do the second and introduce a new PR that should be merged in before this one. Edit: |
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
db90555
to
b2411a3
Compare
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
fd128ec
to
0ef95cd
Compare
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@Karsten1987 this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would very much prefer if we tackle the removal of get_metadata()
from the storage interfaces in this PR as well. That would make at least the writer API complete in a sense to support bag splitting.
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
@@ -91,7 +91,9 @@ class StorageTestFixture : public TemporaryDirectoryFixture | |||
std::unique_ptr<rosbag2_storage::storage_interfaces::ReadWriteInterface> writable_storage = | |||
std::make_unique<rosbag2_storage_plugins::SqliteStorage>(); | |||
|
|||
writable_storage->open(temporary_dir_path_); | |||
auto db_file = rosbag2_storage::FilesystemHelper::concat({temporary_dir_path_, "rosbag"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why that change? Why appending rosbag
to the filename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqliteStorage
is being invoked directly here, so db_file
needs to be the path to the actual database storage. Before this change, it was expecting the base folder since storage also interacted with the metadata file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it then be appended with .db3
? It seems like you do this for the some of the tests below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, looking at this again, I don't understand why rosbag
is essential here.
..._storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
std::string bag_path_; | ||
std::string database_path_; | ||
std::string storage_path_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between these three variables now? What's the difference between database_path_
and storage_path_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writer
consumes bag_path_
, SqliteStorage
consumes storage_path_
, and SqliteWrapper
can consume database_path_
This is related to what I commented here: #185 (comment).
The e2e test in rosbag2_tests
opens the database either through Writer
, SqliteStorage
, or SqliteWrapper
.
SqliteWrapper
hasn't changed and expects the path to the db, which is database_path_
SqliteStorage
now also expects the db as well, but without the extension since that may be plugin-specific. This is storage_path_
Writer
expects the base path, which is bag_path_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqliteStorage now also expects the db as well, but without the extension since that may be plugin-specific. This is storage_path_
I don't think I understand this. In the metadata.yaml, there should be a path to an existing file listed in relative_file_paths
. This filepath should then be passed to a Reader
which opens it through the appropriate plugin. The Sqlite storage plugin should thus be able to handle a file which ends in .db3
.
39b20fd
to
f8872d7
Compare
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
f8872d7
to
b16a8bd
Compare
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
@@ -91,7 +91,9 @@ class StorageTestFixture : public TemporaryDirectoryFixture | |||
std::unique_ptr<rosbag2_storage::storage_interfaces::ReadWriteInterface> writable_storage = | |||
std::make_unique<rosbag2_storage_plugins::SqliteStorage>(); | |||
|
|||
writable_storage->open(temporary_dir_path_); | |||
auto db_file = rosbag2_storage::FilesystemHelper::concat({temporary_dir_path_, "rosbag"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it then be appended with .db3
? It seems like you do this for the some of the tests below as well.
std::string bag_path_; | ||
std::string database_path_; | ||
std::string storage_path_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqliteStorage now also expects the db as well, but without the extension since that may be plugin-specific. This is storage_path_
I don't think I understand this. In the metadata.yaml, there should be a path to an existing file listed in relative_file_paths
. This filepath should then be passed to a Reader
which opens it through the appropriate plugin. The Sqlite storage plugin should thus be able to handle a file which ends in .db3
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have plans on removing the get_metadata
functions on the interfaces? I feel this is one of the most important changes for the set of PRs as it will contain changes to reader and writer api simultaneously.
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
@@ -91,7 +91,9 @@ class StorageTestFixture : public TemporaryDirectoryFixture | |||
std::unique_ptr<rosbag2_storage::storage_interfaces::ReadWriteInterface> writable_storage = | |||
std::make_unique<rosbag2_storage_plugins::SqliteStorage>(); | |||
|
|||
writable_storage->open(temporary_dir_path_); | |||
auto db_file = rosbag2_storage::FilesystemHelper::concat({temporary_dir_path_, "rosbag"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, looking at this again, I don't understand why rosbag
is essential here.
...storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/storage_test_fixture.hpp
Show resolved
Hide resolved
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
22c6e6e
to
3197d1b
Compare
@zmichaels11 do you mind not force pushing after a round of reviews? That makes it easier for me to keep track of individual changes. |
sorry about that |
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_info_interface.hpp
Show resolved
Hide resolved
..._storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
..._storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
…ts with READ_WRITE Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now to me.
The only thing I would like to see here now is a test which tests for multiple files to be written and correctly mentioned in the metadata file.
I assume further, that the removal of get_metadata()
will be done in another PR?
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
..._storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp
Outdated
Show resolved
Hide resolved
Nevermind, did compile my workspace completely. |
Not sure I completely understand this. The command line option doesn't really play a role in this new PR. I do believe we (just) need a system test in ros2_tests which basically just opens a Writer instance, manually adds a couple of messages and verifies that all messages are correctly stored across various bag files. That tests has to be successful for all provided storage implementations.
This makes sense to me for validating the splitting logic within the Writer itself. |
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Added a unit test that validates that Writer is splitting and the bagfiles increment correctly. |
I took a crack at that and I think its going to require enough effort to justify a new PR. Most of what you suggested works, except that there isn't a cross-platform way to create directory structures in c++. I think that we should wrap up this PR, port I have added a unit test to |
CI: The changes for the writer API look good to me now. However, I am unable to play a bag file with this PR. I presume it's because the changes for loading the metadata appropriately on the reader side are not part of this PR.
Pointing me to the direction of if (is_read_write(io_flag)) {
relative_path_ = uri + FILE_EXTENSION;
// READ_WRITE requires the DB to not exist.
if (database_exists(relative_path_)) {
throw std::runtime_error(
"Failed to create bag: File '" + relative_path_ + "' already exists!");
}
} else { // APPEND and READ_ONLY
relative_path_ = uri;
// APPEND and READ_ONLY require the DB to exist
if (!database_exists(relative_path_)) {
throw std::runtime_error(
"Failed to read from bag: File '" + relative_path_ + "' does not exist!");
}
} which lets me believe that the relative path is still set to the base folder path and the actual metadata is not being read. As a side note: This also means that How do you guys want to proceed with this? I can't merge this PR as-is as this would break rosbag2 because of not being able to replay bag files. |
I had @piraka9011 look at this since he has context on the Reader. I'm going to look into making |
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@Karsten1987 I think we resolved your issues. |
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
Show resolved
Hide resolved
storage_ = storage_factory_->open_read_only(storage_options.uri, storage_options.storage_id); | ||
metadata_ = metadata_io_->read_metadata(storage_options.uri); | ||
storage_ = storage_factory_->open_read_only( | ||
metadata_.relative_file_paths[0], storage_options.storage_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should at least check if relative_file_paths
is non empty otherwise this will lead to a segfault.
Except that one outstanding comment, that looks good to me. |
@Karsten1987 added a check! |
@piraka9011 I was actually about to do the same, except throw instead. metadata_.relative_file_paths = {"dummy path"}; |
I followed the same convention as the next check for the topics in L#58. Updating tests. |
An immediate return is fine, but I think in both cases we should log a warning saying that either no concrete files are found or no topics are listed. |
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
092999f
to
bc92f10
Compare
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@zmichaels11 Updated tests |
This is part of an effort to rework PR #158 into multiple, smaller PRs.
Changes
open
inBaseIOInterface
now must point to the storage fileWriter
is responsible for writing and generatingBagMetadata
Writer
rolls over to a new storage when the bagfile exceedsmax_bagfile_size
by opening a new storage.Dependent PRs
Issues