Adding db directory creation to rosbag2_cpp #450

Marwan99 · 2020-07-01T15:21:09Z

Please let me know if you think this functionality should be implemented elsewhere instead of storage implementation.

SteveMacenski · 2020-07-06T19:29:49Z

@Karsten1987 can you review this (if in your domain)? This is blocking some porting efforts and seems relatively small

Karsten1987 · 2020-07-07T16:45:26Z

thanks a lot for picking this up. However, I think this logic makes more sense within rosbag2_cpp, as we're already dealing with file paths in there and take certain assumptions.

Karsten1987 · 2020-07-07T18:10:00Z

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

@@ -97,6 +97,16 @@ void SequentialWriter::open(

  const auto storage_uri = format_storage_uri(base_folder_, 0);

+  rcpputils::fs::path db_path(storage_uri);
+  if (!db_path.parent_path().is_directory()) {


what happens if the directory exists already?

I think the logic should be inverted here: There should be an exception being raised the moment the directory exists already. That's the current behavior in ros2bag. Simply because we don't want to overwrite any files.

That makes sense, should be changed now.

Karsten1987 · 2020-07-07T18:12:34Z

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

@@ -97,6 +97,16 @@ void SequentialWriter::open(

  const auto storage_uri = format_storage_uri(base_folder_, 0);

+  rcpputils::fs::path db_path(storage_uri);


why not using base_folder_? That should avoid the need to verify the parent_path of it.

Karsten1987 · 2020-07-14T20:48:47Z

friendly ping @Marwan99

Marwan99 · 2020-07-14T21:04:47Z

Sorry for the radio silence, just picked it back again today.

Karsten1987

This looks good to me.

I think you'd need to address the resulting test failures though as well, mainly the tests within rosbag2_cpp, but I have seen other packages fail as well, i.e. rosbag2_tests.
You can verify the tests with colcon test --packages-select-regex rosbag2.

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

Karsten1987 · 2020-07-14T23:02:08Z

Thinking a bit more about this, I would assume we have to delete the logic within ros2bag here as this would conflict directly with the logic in this PR.

Marwan99 · 2020-07-16T15:00:03Z

@Karsten1987 ready for review

Karsten1987

Thanks, this looks already quite good. One more round of small comments inline.

Karsten1987 · 2020-07-20T23:26:52Z

rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp

@@ -47,6 +47,9 @@ class SequentialWriterTest : public Test
    storage_options_ = rosbag2_cpp::StorageOptions{};
    storage_options_.uri = "uri";

+    rcpputils::fs::path dir(storage_options_.uri);
+    rcpputils::fs::remove(dir);


I actually think that this should be remove_all in case the directory is not empty.

rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp

Karsten1987 · 2020-07-20T23:28:54Z

rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

@@ -559,7 +559,7 @@ TEST_F(RecordFixture, record_fails_gracefully_if_bag_already_exists) {
 TEST_F(RecordFixture, record_fails_if_both_all_and_topic_list_is_specified) {
  internal::CaptureStderr();
  auto exit_code =
-    execute_and_wait_until_completion("ros2 bag record -a /some_topic", temporary_dir_path_);
+    WEXITSTATUS(std::system("ros2 bag record -a /some_topic"));


why this change? Same below

These tests were failing and that fix gave me a placebo of fixing the root cause, apparently the issue was somewhere else. Double checked local and that is not needed anymore, should be reverted back now.

Marwan99 · 2020-07-21T00:09:46Z

Just curious, why is the CI not happy with rcpputils::fs::remove_all? Is it using a cached or released version of rcpputils?

Karsten1987 · 2020-07-21T00:25:01Z

the remove_all functionality is not yet released into rolling. It is present on the current master branches though, so when we trigger CI on ci.ros2.org it won't be failing.

Karsten1987 · 2020-07-21T00:28:08Z

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2020-07-21T04:18:24Z

@Marwan99 do you mind rebasing your branch on top of master?

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

…_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 · 2020-07-21T15:42:34Z

Sure, just to confirm I will need to force push after rebasing. Is that okay?

Karsten1987 · 2020-07-21T16:16:58Z

Sure, just to confirm I will need to force push after rebasing. Is that okay?

yes!

emersonknapp · 2020-07-21T19:40:33Z

Linux
Linux-aarch64
macOS
Windows

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp

* added db directory creation to storage factory Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * moved db directory creation to rosbag2_cpp Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * rasing exception if dir already exists Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * removed dir creation from record.py, added dir creation to sequential_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixed failing tests Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixing review comments Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * Apply suggestions from code review Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>

* added db directory creation to storage factory Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * moved db directory creation to rosbag2_cpp Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * rasing exception if dir already exists Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * removed dir creation from record.py, added dir creation to sequential_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixed failing tests Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * fixing review comments Signed-off-by: Marwan Taher <marokhaled99@gmail.com> * Apply suggestions from code review Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com> Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Marwan99 force-pushed the db-dir-creation branch from 92dccf5 to 79b44f3 Compare July 1, 2020 23:47

Karsten1987 reviewed Jul 7, 2020

View reviewed changes

Karsten1987 reviewed Jul 14, 2020

View reviewed changes

rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Outdated Show resolved Hide resolved

Marwan99 changed the title ~~Adding db directory creation to storage implementation~~ Adding db directory creation to rosbag2_cpp Jul 17, 2020

Karsten1987 reviewed Jul 20, 2020

View reviewed changes

Marwan99 added 6 commits July 21, 2020 12:21

added db directory creation to storage factory

047c3a3

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

moved db directory creation to rosbag2_cpp

6b64540

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

rasing exception if dir already exists

2adff75

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

removed dir creation from record.py, added dir creation to sequential…

0349e04

…_compression_writer and refactored dir creation in sequential_writer Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

fixed failing tests

ae6fd06

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

fixing review comments

3529fc2

Signed-off-by: Marwan Taher <marokhaled99@gmail.com>

Marwan99 force-pushed the db-dir-creation branch from 9e9db06 to 3529fc2 Compare July 21, 2020 18:18

Karsten1987 approved these changes Jul 21, 2020

View reviewed changes

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp Outdated Show resolved Hide resolved

rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

9df90ff

Karsten1987 merged commit d460c1f into ros2:master Jul 21, 2020

Marwan99 deleted the db-dir-creation branch July 21, 2020 22:30

Karsten1987 mentioned this pull request Jul 23, 2020

SequentialWriter::open failing to create a bag file #448

Closed

homalozoa mentioned this pull request Oct 21, 2022

Check & create database dirs before writing bags #1133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding db directory creation to rosbag2_cpp #450

Adding db directory creation to rosbag2_cpp #450

Marwan99 commented Jul 1, 2020

SteveMacenski commented Jul 6, 2020

Karsten1987 commented Jul 7, 2020

Karsten1987 Jul 7, 2020

Marwan99 Jul 14, 2020

Karsten1987 Jul 7, 2020

Karsten1987 commented Jul 14, 2020

Marwan99 commented Jul 14, 2020

Karsten1987 left a comment

Karsten1987 commented Jul 14, 2020

Marwan99 commented Jul 16, 2020

Karsten1987 left a comment

Karsten1987 Jul 20, 2020

Karsten1987 Jul 20, 2020

Marwan99 Jul 21, 2020

Marwan99 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020 •

edited

Loading

Karsten1987 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020

Marwan99 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020

emersonknapp commented Jul 21, 2020

		@@ -97,6 +97,16 @@ void SequentialWriter::open(

		const auto storage_uri = format_storage_uri(base_folder_, 0);

		rcpputils::fs::path db_path(storage_uri);

Adding db directory creation to rosbag2_cpp #450

Adding db directory creation to rosbag2_cpp #450

Conversation

Marwan99 commented Jul 1, 2020

SteveMacenski commented Jul 6, 2020

Karsten1987 commented Jul 7, 2020

Karsten1987 Jul 7, 2020

Choose a reason for hiding this comment

Marwan99 Jul 14, 2020

Choose a reason for hiding this comment

Karsten1987 Jul 7, 2020

Choose a reason for hiding this comment

Karsten1987 commented Jul 14, 2020

Marwan99 commented Jul 14, 2020

Karsten1987 left a comment

Choose a reason for hiding this comment

Karsten1987 commented Jul 14, 2020

Marwan99 commented Jul 16, 2020

Karsten1987 left a comment

Choose a reason for hiding this comment

Karsten1987 Jul 20, 2020

Choose a reason for hiding this comment

Karsten1987 Jul 20, 2020

Choose a reason for hiding this comment

Marwan99 Jul 21, 2020

Choose a reason for hiding this comment

Marwan99 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020 • edited Loading

Karsten1987 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020

Marwan99 commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020

emersonknapp commented Jul 21, 2020

Karsten1987 commented Jul 21, 2020 •

edited

Loading