-
Notifications
You must be signed in to change notification settings - Fork 253
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
Mark bags that are still being recorded #1597
Comments
@MichaelOrlov @emersonknapp any recollection on why this was omitting from rosbag2 or if there's some gotcha with adding support for this? |
@defunctzombie @tonynajjar I don't know about any prior discussions about having a different suffix for currently writing file. Also, I think that one of the reasons we didn't follow ROS 1 behavior is because we handling the matter of concern in the question
a better way than in ROS 1. The one can subscribe to the topic rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp Lines 321 to 337 in 3032800
and rosbag2/rosbag2_transport/src/rosbag2_transport/recorder.cpp Lines 356 to 376 in 3032800
The key info there is
|
The script in question is the foxglove-agent which is closed-source so unfortunately I don't have the option to adapt it. Either the Foxglove guys would need to do so (@defunctzombie ), or rosbag2 has to introduce a differentiator in the file name as this is the only thing foxglove-agent can ignore. |
One option for ROS users would be a node which listens for these events and moves the "done" recordings to a different folder that is picked up by systems looking for files to upload. Any system that could previously "watch" directories for changes now needs to be more "ROS-native" and be able to hook into the pubsub mechanism to listen for such events. Also raises some questions about what happens if such a script is started after recording nodes or re-started. Filesystem level indicators (like file extensions done in ROS1) are in some ways a more robust approach. |
I need to clarify. In general, I don't mind having a feature when a file that is currently in recording to be named with |
Perhaps a simple |
That is not clear to me. We certainly shall not try to copy the finished |
Rosbags in ROS 2 are actually folders with a meta file and a database right? We could place an empty file called Might be easier on bash/python like scripts which don't have ROS capability. |
@Timple Thanks for the clarification. rosbag2/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Lines 162 to 164 in 34ccb39
To get the current filename we can use storage_->get_relative_file_path() Then will need to try to delete the .active file in close() methodrosbag2/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Lines 182 to 188 in 34ccb39
and do delete and create a new .active file in the split_bagfile() methodrosbag2/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp Lines 331 to 337 in 34ccb39
|
IMO it is better to use |
@defunctzombie I tried that and while that generally can work, it fails (or requires more implementation effort) in corner cases like when shutting down the system: although rosbag2 will conclude and close the bag being recorded, the script won't receive the WriteSplit event and therefore not move the last bag to the "done" folder. I'm not saying this can't be solved/workedaround but it has enough complexity to say that that's not something any ROS user can simply do -> better solve this at rosbag2 or foxglove-agent's level |
@Timple @MichaelOrlov The problem I see with this is that sometimes the bag folder contains more than one bag file, e.g. when splitting recording by duration. An |
I think you're missing the structure. Rosbags are folders. They are also within folders.
|
Thanks for the example, I'll give you a counter example:
In this case, the directory would look like that:
At that time, only |
Ahaaa, I did not know that split bagfiles actually ended up in the same directory. (Sorry, should/could have verified). But this is kind of strange right? The 'total' bagfile is still open. I know we can treat mcap files isolated from their folders, but that's a whole different issue perhaps? But I don't actually mind the direction we end up with 🙂 . So alternatives are fine for me! |
FWIW I take a different view here. I do not consider the folder a "bag" - to me it is simply that - a file system folder that holds some files. I consider the individual file (the mcap) the "bag" file. I've always found it strange that this "folder" layer was introduced; for me it adds a layer of indirection and nomenclature I find confusing and unnecessary. |
Adding My idea is to mark as "locked" all the files in the directory, until recording is done. |
File locks are not a bad idea, but no |
funny, I would have sweared that boost is a common depoendency in the core of ROS. No problem 😄 |
If this was only for the application itself to manage this could be ok but if the intent is to have outside applications or folks listing the directory easily understand the situation I think |
IMO, i second this with renaming the file without creating extra file nodes. sending the data / logging files to the server or cloud once it is ready is not application specific behavior, this could be also nicer for rosbag2 to go with. we can lock the file either segment or file descriptor, but sounds overkill unless we actually have racy condition to read/write, i guess. btw, i have a question about |
Signed-off-by: Davide Faconti <[email protected]>
Signed-off-by: Davide Faconti <[email protected]>
@fujitatomoya @facontidavide @defunctzombie The proposed solution with renaming the currently writing file doesn't fit very well with the current design of the rosbag2, specifically when we are using per-file compression. Think about a situation when we do split with file compression.
Please note that currently, we have a buggy behavior with file compression when we are sending notifications about the bag_split event via messages. i.e. we are sending notifications when bag file is closed after the split not when it has been compressed. I have tried to fix it in the #1643 However, don't have time to write a unit tests. |
👍 agree, and thanks for sharing insights! all that said, i think the completion event (renaming system calls on this event) should be including compression operation if compression is required to process. this is just because file is not ready to use yet. |
Description
In ROS1, a bag that was still being recorded was suffixed with
.active
, it would be nice if in rosbag2 there was something similar. An example use case is a scripts that uploads bags to a server automatically - that script needs to know not to upload a bag if it's still recording.The text was updated successfully, but these errors were encountered: