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

SQLite storage optimized by default #568

Merged
merged 7 commits into from
Jan 8, 2021

Conversation

adamdbrw
Copy link
Collaborator

@adamdbrw adamdbrw commented Nov 24, 2020

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.

image(2)

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

…se former behavior

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@adamdbrw adamdbrw requested a review from a team as a code owner November 24, 2020 13:13
@adamdbrw adamdbrw requested review from jaisontj, dabonnie, Karsten1987 and mjeronimo and removed request for a team November 24, 2020 13:13
@adamdbrw
Copy link
Collaborator Author

@mjeronimo and/or @Karsten1987, could you review this one?

ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved
…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"
Copy link
Contributor

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".

Copy link
Collaborator Author

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

https://www.sqlite.org/pragma.html#pragma_synchronous

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@mjeronimo
Copy link
Contributor

mjeronimo commented Nov 30, 2020

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

Copy link
Collaborator

@emersonknapp emersonknapp left a 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.

ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
ros2bag/ros2bag/verb/record.py Show resolved Hide resolved
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@adamdbrw
Copy link
Collaborator Author

Fixing the merge - went a bit sideways

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@emersonknapp
Copy link
Collaborator

emersonknapp commented Jan 5, 2021

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
BUILD args: --packages-up-to ros2bag rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
TEST args: --packages-select ros2bag rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport
Job: ci_launcher

ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7391

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

@emersonknapp emersonknapp requested review from emersonknapp and removed request for jaisontj and dabonnie January 5, 2021 03:15
- 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>
@adamdbrw
Copy link
Collaborator Author

adamdbrw commented Jan 8, 2021

@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>
Copy link
Collaborator

@emersonknapp emersonknapp left a 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

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/b8abd49ac4aa32d70feee3fa3b3b06b3/raw/34a8fb38e8af97a7e3f68337d715b0a0c1411b52/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
TEST args: --packages-select ros2bag rosbag2_storage rosbag2_storage_default_plugins rosbag2_transport rosbag2_tests
Job: ci_launcherci_launcher ran: https://ci.ros2.org/job/ci_launcher/7421

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

@emersonknapp emersonknapp merged commit aeae096 into master Jan 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the sqlite_storage_optimized_by_default branch January 8, 2021 23:01
adamdbrw added a commit that referenced this pull request Jan 14, 2021
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
* Use optimized pragmas by default in sqlite storage. Added option to use former behavior

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants