-
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
SQLite storage optimized by default #568
Conversation
…se former behavior Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@mjeronimo and/or @Karsten1987, could you review this one? |
…ettings Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
// resilient_storage_writing should replace "some_value" with "wal | ||
EXPECT_EQ(writable_storage->get_storage_setting("journal_mode"), "wal"); | ||
|
||
// resilient_storage_writing should replace "another_value" with "2" |
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.
Comment does not match code. The comment mentions setting "another_value" to "2", while the code is checking against "1".
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.
The correct value for default is NORMAL (returned as 1), so the comment is mistaken (commented before I checked) and the code is right.
Fixed
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
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'm back from long holiday weekend - sorry for the slow response. My main comment is the structural concern of making the storage settings profiles more extensible.
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Fixing the merge - went a bit sideways |
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Running for evidence - we can override my comments for now if @mjeronimo you'd rather move forward this way and have us address that stuff later. Though @adamdbrw I would like to address some of the smaller semantics issues I commented on, if we can Gist: https://gist.githubusercontent.com/emersonknapp/c32bfc13fc1341e9bdaa088b4ef23bf1/raw/34a8fb38e8af97a7e3f68337d715b0a0c1411b52/ros2.repos ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7391 |
- changed api to --storage-preset-profile - switched logic to prioritize config file settings over preset - added another test Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@emersonknapp @mjeronimo I addressed the API issue - should remain the same now and we are free to add new presets (or change the way we acquire presets from a simple static list as it is currently). Also, config file is now overriding the preset as you suggested. I also added explanation that the crash-caused corruption risk only applies to current bagfile if splitting is on. Windows yellowness does not look as if is is related - "Cannot find 'uncrustify' executable" is this good to be merged now? |
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
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.
LGTM! Thanks for the changes. Running CI now
Gist: https://gist.githubusercontent.com/emersonknapp/b8abd49ac4aa32d70feee3fa3b3b06b3/raw/34a8fb38e8af97a7e3f68337d715b0a0c1411b52/ros2.repos |
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
With optimized settings applied, performance of storage writing is significantly increased. This reduces the problem of messages being lost when there is a high throughput of recorded data.
Note that before this PR optimization is already accessible to users through
--storage-config-file settings
. This PR makes storage optimization the default.Includes an option to use former settings:
--resilient-storage-writing