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

Use directory as bagfile and add additonal record options #43

Merged
merged 4 commits into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@greimela-si
Copy link
Contributor

commented Oct 11, 2018

Store rosbags using a folder, not a file.
This folder contains all files created by the storage backend.

Add record options --output to state the output file and --storage to select the storage backend.
Default: Use the current timestamp as bag name and sqlite3 as storage backend.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@Karsten1987
Copy link
Contributor

left a comment

One little comment about the flexibility of the bag files when manually copied around.

return
return 'Invalid choice: Can not specify topics and -a at the same time.'

uri = args.output if args.output else datetime.datetime.now().strftime("%Y_%m_%d-%H_%M_%S")

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 11, 2018

Contributor

I would recommend prefixing the timestamped folder. Something like rosbag2_[recording_]_YYYY_MM_DD-HH_MM_SS

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 12, 2018

Author Contributor

Good idea, we will use rosbag2_YYYY_MM_DD-HH_MM_SS.

@@ -168,6 +175,11 @@ void SqliteStorage::fill_topics_and_types()
}
}

std::string SqliteStorage::get_database_name(const std::string & uri)
{
return rosbag2_storage::FilesystemHelper::get_folder_name(uri) + ".db3";

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 11, 2018

Contributor

What happens if I put a .db3 file in a new folder and want to open this one? LIke shifting backfiles around?

.../my_new_folder/my_old_bag.db3

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 12, 2018

Author Contributor

At the moment it is required that the database file is named like the bag folder.

We can change this behavior as soon as the metadata file is available (PR #45).
After that we should use the file path provided by the metadata file to locate the database file.
This path then has to be adjusted after renaming/moving the database file.

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 12, 2018

Author Contributor

Like shifting bagfiles around?

Generally, we should consider the folder being the bagfile.
So the user should ideally never move/rename/modify the files inside a bag, except when he knows exactly what he does.

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 15, 2018

Contributor

I somewhat agree with it, but I also don't see a strong reason why we should enforce it. I could imagine having an old bagfile around and put it into a new folder. Then I should still be able to load this file.
Also in the long term run, whenever there are split rosbag files, this strong naming match won't hold anymore as every individual bag file will have some sort of pre-/postfix attached to it.

Just for now, I don't see why we should enforce this on code level. Given a URI, I should always be able to extract the folder name independant from the database file.

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 16, 2018

Author Contributor

I somewhat agree with it, but I also don't see a strong reason why we should enforce it. I could imagine having an old bagfile around and put it into a new folder. Then I should still be able to load this file.

The problem is that we do not have any metadata at this point.
So I see no way how rosbag2 should be able to know that a file in a folder can be understood as a database file.
That's the reason why we enforce this naming scheme for now.
As soon as the info PR lands we have the metadata file which contains the relative file path to the database and can use this path to locate the database.

Also in the long term run, whenever there are split rosbag files, this strong naming match won't hold anymore as every individual bag file will have some sort of pre-/postfix attached to it.

This is also solved using the relative file paths from the metadata file.
This case is the reason why we added the relative_file_paths array to the metadata file.

Just for now, I don't see why we should enforce this on code level. Given a URI, I should always be able to extract the folder name independant from the database file.

Since the database file naming is solely handled by the storage plugin I don't see how this can be achieved through the general rosbag2 interface.


using namespace ::testing; // NOLINT

#ifdef _WIN32

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 11, 2018

Contributor

with all that #ifdef, does it make sense to include two different helper files (one for win, one for unix) and only separate them once at the beginning?

#ifdef _WIN32
#include process_execution_helpers_windows.hpp
#else 
#include process_execution_helpers_unix.hpp
#endif 

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 12, 2018

Author Contributor

The current implementation has one big advantage:
It hides the fact that processes behave differently on windows and unix.

The includer therefore does not have to consider OS details and simply includes the only header available.

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 15, 2018

Contributor

Sorry, what I meant was having three files:

  • process_execution_helpers.hpp
  • process_execution_helpers_windows.hpp
  • process_execution_helpers_unix.hpp

The first one having the code snippet above included.

This comment has been minimized.

Copy link
@greimela-si

greimela-si Oct 16, 2018

Author Contributor

Ah, yes, that's a good idea, I added it.

@greimela-si greimela-si force-pushed the bsinno:feature/directory_as_bagfile branch from 4786ac4 to ea85da2 Oct 16, 2018

GH-109 Improve e2e-test process handling on linux
- Use SIGINT instead of SIGTERM which is the correct equivalent to
  pressing Ctrl-C within a shell
- Use exec instead of system inside the forked test child process.
- Check the return code of the child after the waitpid instead of
  after the call to system.
Using system inside a child process is unnecessary as system forks
again. Whether the actual "ros2 bag" process was put in the same
process group seemed to be shell dependent. Exec avoids these issues.
@Martin-Idel-SI

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

New CI:

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

@Karsten1987 Karsten1987 merged commit e395391 into ros2:master Oct 17, 2018

@Karsten1987 Karsten1987 removed the in review label Oct 17, 2018

@Martin-Idel-SI Martin-Idel-SI deleted the bsinno:feature/directory_as_bagfile branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.