Change default cache size for sequential_writer to a non zero value #533
Conversation
| @@ -67,6 +67,7 @@ TEST_F(RecordFixture, record_end_to_end_test_with_zstd_file_compression) { | |||
| cmd << "ros2 bag record" << | |||
| " --compression-mode file" << | |||
| " --compression-format zstd" << | |||
| " --max-cache-size 0" << | |||
There was a problem hiding this comment.
These tests assume the cache size to be 0 [disk write always] and therefore this flag is now required for them to pass.
| @@ -40,8 +40,6 @@ void Reader::open(const std::string & uri) | |||
| rosbag2_cpp::StorageOptions storage_options; | |||
| storage_options.uri = uri; | |||
| storage_options.storage_id = "sqlite3"; | |||
| storage_options.max_bagfile_size = 0; // default | |||
There was a problem hiding this comment.
I believe this assignment in unnecessary and keeps the code clean and consistent
[Note: if we want to keep this then storage_options.max_bagfile_duration = 0, is something we should add to this.]
There was a problem hiding this comment.
Yes - I think you are right. It might also make sense to move the "sqlite3" default value to the struct definition as well. No need to do that right now though
There was a problem hiding this comment.
I assume this depends on #533 - you should probably mark it as such in the PR description so that we remember to merge this only after the other.
[Edited] Well, technically, it does not depend on #530 since this just changes the default value for the cache size and handles the test for them as well. But I want to merge this after #530 and have added the dependency in the comment. |
|
This would change the default cache to one million messages if it was merged before #533 - I think that is probably an unreasonable amount, therefore I'd call it a dependency. E.g. you are storing an IMU message, this could easily make the default cache end up something like >300MB |
True, agreed. That makes sense. |
Signed-off-by: Jaison Titus <[email protected]>
Signed-off-by: Jaison Titus <[email protected]>
Signed-off-by: Jaison Titus <[email protected]>
Signed-off-by: Jaison Titus <[email protected]>
open Signed-off-by: Jaison Titus <[email protected]>
jaisontj
force-pushed
the
jaisontj/fix-464-change-default
branch
from
756f331 to
044c664
Compare
|
Running CI
|
|
Looks like xmllint is failing on all packages currently in the nightly build - so the instability is not related to this PR. |
…533) * Change default value for max-cache-size to 1MB Signed-off-by: Jaison Titus <[email protected]>
…533) * Change default value for max-cache-size to 1MB Signed-off-by: Jaison Titus <[email protected]>
…os2#533) * Change default value for max-cache-size to 1MB Signed-off-by: Jaison Titus <[email protected]>
MERGE AFTER PR #530
Currently, the default cache size for the
sequential_writeris 0 which leads to a write to disk on everywritecall. This PR aims at changing that default value to 1MB [or any other value greater than 0]Addresses #464
A note on changing the default value: This PR focuses on changing the default cache size when rosbag2 is invoked using the CLI. As for situations where a user might directly use the library, there is no mention of default but a note that a value of 0 will force all writes to disk.
For quick reference, these are all the files where
max_cache_sizeis used/set: