read yaml config file #497

Karsten1987 · 2020-08-07T22:24:54Z

implements to read a config file (.yaml) tailored for sqlite3. As for now, I've only implemented to read pragmas in the style of:

read:
  pragmas: ["schema_version"]
write:
  pragmas: ["synchronous = NORMAL", "journal_mode = WAL"]

I am struggling a bit to write tests for this as I have a hard time to introspect if these pragmas are actually affective. I can't really test for wrong pragmas either as according to the sqlite documentation "No error messages are generated if an unknown pragma is issued."

Karsten1987 · 2020-09-01T19:33:09Z

this is rebased on top of the latest changes in #493

emersonknapp

This looks good - a few stylistic notes if you want to take them

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

adamdbrw · 2020-10-12T15:56:36Z

In terms of testing - perhaps querying if the pragma is set would suffice?

PRAGMA pragma_name;

should return the current value of the specific pragma setting.

adamdbrw · 2020-10-15T10:58:38Z

@Karsten1987 I would like to suggest some changes as well as discuss overall design. I don't have access rights to comment or push changes to the branch. How do we proceed?

Are you going to apply pending review suggestions by @emersonknapp? It would be helpful to resolve them in any way.
I can open a PR to this branch with my suggestions and fixes. Would that PR be the right place to discuss design?
Would you like to rebase the parent branch before we proceed?

Meanwhile, I will work on it locally

Karsten1987 · 2020-10-19T23:04:27Z

@adamdbrw you should have write access now to this repo. Feel free to simply push ontop of this branch. I've rebased and hopefully addressed Emerson's comments.

emersonknapp

lgtm

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

Karsten1987 · 2020-10-28T16:55:08Z

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2020-10-28T19:25:11Z

Linux
Linux-aarch64
macOS
Windows

adamdbrw · 2020-10-29T11:36:56Z

As I am documenting the use of storage configuration parameter, I am struggling to find a good example of a useful PRAGMA setting for read: section in the yaml.
Do we have an example of how this could be useful, considering we can't do much that affects the db and we don't actually have a mechanism to query (read) pragma settings and return them to the user (even for information purposes)?

If not, I would change the --storage-config-file option to be only available to ros2 bag record and simplify yaml section from write/pragmas to pragmas.

Also, we should decide on the default values for journal_mode and synchronous settings. This would have significant impact on user experience in high performace scenarios, but I am also not sure how the db behaves with these settings when a crash occurs, and how common crashes are for users atm.

pijaro · 2020-10-29T12:08:00Z

I think that --storage-config-file is semantically correct for both play and record so we should keep it to simplify the cli interface, even if one scenario is currently lacking utility.

emersonknapp · 2020-10-29T20:07:52Z

I think that --storage-config-file is semantically correct for both play and record so we should keep it to simplify the cli interface, even if one scenario is currently lacking utility.

I agree with this, it's more flexible to allow for future use cases

adamdbrw · 2020-11-02T11:18:56Z

I had to force push to fix DCO issues

Adam Dąbrowski as author vs Adam Dabrowski (ą -> a) in signoff. I have been using plain "a".
Tomoya.Fujita vs tomoya in author/signoff mismatch (not sure how this one happened either).

Also, added some documentation earlier (could be more difficult to notice because of force pushes).
One pending decision - whether to change defaults for journal_mode and synchronous.

Karsten1987 · 2020-11-02T16:13:41Z

Tomoya.Fujita vs tomoya in author/signoff mismatch (not sure how this one happened either).

This change seems unrelated to your PR and should not happen. I don't appreciate changing history of committers not participating in this PR.

adamdbrw · 2020-11-02T17:00:57Z

@Karsten1987 I assumed I was only reversing what happened as a side effect during a rebase I performed, since the original change was merged in already. Is there a suggestion how to handle this at this moment?

emersonknapp · 2020-11-02T22:18:40Z

You should be able to solve your problem with a proper rebase on master with no merges.

One way to get the history quite clean, if you don't care about any intermediate history on this PR, is to just git reset --mixed origin/master and make a single new commit that contains all changes.

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

- includes documentation of storage-config-file option - includes adaptation of benchmarking script to cache size semantic change - tests with valid and invalid storage file Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

adamdbrw · 2020-11-03T14:34:22Z

@emersonknapp thanks for the tip, I used your approach but also cherry-picked @Karsten1987 commits so we have contributions properly attributed. Now, this should be much better - I preserved authorship of commits and at the same time merged all mine into what looks to me like a proper, targeted change. Let me know if this looks good to you.

I am looking into Windows build unstability - it seems that opening with a valid storage config file throws an exception.
By the look of it, this could be an issue with newline character & yaml parsing - @Karsten1987 could you check that for me? I would need to setup a Windows machine which is time consuming.

One place to look at is this line in test_sqlite_storage.cpp. I thought that newlines should be correctly converted to \n\r on Windows on flush, but maybe that's not the case:
const auto valid_yaml = "write:\n pragmas: [\"journal_mode = MEMORY\"]\n";

Karsten1987

This looks principally good to me. Just a few outstanding comments inline.

Running CI on this:

Linux
Linux-aarch64
macOS
Windows

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

Karsten1987 · 2020-11-09T20:51:19Z

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

+        {"journal_mode", pragma_assign + "WAL"},
+        {"synchronous", pragma_assign + "NORMAL"}


I am not sure how we decided on the default values in the end, but that's the non-optimized pragmas, right? Have we hereby decided that these are the default values?

If I recall correctly, we decided to change the defaults in a separate PR (along with changing the default split size to something else than 0 and including information about possible consequences). I can also add this change to current PR, doesn't make a difference to me. Let me know what is better.

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

pijaro · 2020-11-10T15:24:45Z

I've inspected windows issues with rosbag2_storage_defatult_plugins tests. It looks like there is a problem with removing a temp dir when the storage throws an error in the throws_on_ivalid_pragma_in_config_file test (btw, typo in 'invalid' 😄 ). Temp dir can’t be removed because of win32 ERROR_SHARING_VIOLATION: “The process cannot access the file because it is being used by another process.”. You can confirm that by checking return value of SHFileOperation(…) in temporary_directory_fixture.hpp from rosbag2_test_common package.

Dirty workaround is to change tests order (move the test whith EXPECT_THROW to the end) - but still, some folders will remain in windows temp location.

- change pragma tests order as a workaround to fix tests issue on Windows: exception throwing tests don't clean temp folders Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

adamdbrw · 2020-11-10T16:13:40Z

This should also pass Windows tests now

Karsten1987

CI

Linux
Linux-aarch64
macOS
Windows

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

adamdbrw · 2020-11-12T13:12:46Z

@Karsten1987: With all review remarks applied, are we good to merge this now?

Karsten1987 · 2020-11-13T01:56:01Z

This looks good to me. Thanks for iterating with me. Just fyi, I can't formerly approve this PR as it's actually still opened under my name, but I just believe that @emersonknapp's approval still counts ;-)

CI for completness:

Linux
Linux-aarch64
macOS
Windows

* read yaml config file Signed-off-by: Karsten Knese <karsten@openrobotics.org> * address review comments Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * Sqlite storage pragmas handling and validation - includes documentation of storage-config-file option - includes adaptation of benchmarking script to cache size semantic change - tests with valid and invalid storage file Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Applied review remarks Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * - Typo fix - change pragma tests order as a workaround to fix tests issue on Windows: exception throwing tests don't clean temp folders Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Applied review: moved some parsing and key extraction to parse function Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> Co-authored-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

* read yaml config file Signed-off-by: Karsten Knese <karsten@openrobotics.org> * address review comments Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com> * Sqlite storage pragmas handling and validation - includes documentation of storage-config-file option - includes adaptation of benchmarking script to cache size semantic change - tests with valid and invalid storage file Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Applied review remarks Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * - Typo fix - change pragma tests order as a workaround to fix tests issue on Windows: exception throwing tests don't clean temp folders Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> * Applied review: moved some parsing and key extraction to parse function Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai> Co-authored-by: Adam Dabrowski <adam.dabrowski@robotec.ai> Signed-off-by: Emerson Knapp <eknapp@amazon.com>

Karsten1987 force-pushed the storage_config branch from 5cdac32 to 2219919 Compare September 1, 2020 16:40

Karsten1987 force-pushed the sqlite3_storage_config branch from 3cc07d3 to ffca12e Compare September 1, 2020 16:59

emersonknapp approved these changes Sep 4, 2020

View reviewed changes

Karsten1987 force-pushed the sqlite3_storage_config branch from ffca12e to 3d86a14 Compare October 19, 2020 22:43

Karsten1987 requested a review from a team as a code owner October 19, 2020 22:43

Karsten1987 requested review from jaisontj and jikawa-az and removed request for a team October 19, 2020 22:43

emersonknapp approved these changes Oct 26, 2020

View reviewed changes

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp Outdated Show resolved Hide resolved

delete-merged-branch bot deleted the branch master October 28, 2020 16:44

Karsten1987 changed the base branch from storage_config to master October 28, 2020 16:44

Karsten1987 force-pushed the sqlite3_storage_config branch from 446d339 to dbc311a Compare October 28, 2020 16:49

adamdbrw force-pushed the sqlite3_storage_config branch 2 times, most recently from ac0447d to e32edbb Compare November 2, 2020 11:16

Karsten1987 and others added 3 commits November 3, 2020 15:13

read yaml config file

3dbcc16

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

address review comments

9191a3b

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>

Sqlite storage pragmas handling and validation

6e1fd86

- includes documentation of storage-config-file option - includes adaptation of benchmarking script to cache size semantic change - tests with valid and invalid storage file Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

adamdbrw force-pushed the sqlite3_storage_config branch from 4aba1a8 to 6e1fd86 Compare November 3, 2020 14:29

Karsten1987 commented Nov 9, 2020

View reviewed changes

Applied review remarks

377c564

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

- Typo fix

fbc3a5d

- change pragma tests order as a workaround to fix tests issue on Windows: exception throwing tests don't clean temp folders Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

Karsten1987 commented Nov 10, 2020

View reviewed changes

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp Outdated Show resolved Hide resolved

Applied review: moved some parsing and key extraction to parse function

7ad028f

Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

Karsten1987 merged commit 4bcb631 into master Nov 13, 2020

delete-merged-branch bot deleted the sqlite3_storage_config branch November 13, 2020 05:17

emersonknapp mentioned this pull request Jun 24, 2021

Backport performance improvements to Foxy #657

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read yaml config file #497

read yaml config file #497

Karsten1987 commented Aug 7, 2020

Karsten1987 commented Sep 1, 2020

emersonknapp left a comment

adamdbrw commented Oct 12, 2020

adamdbrw commented Oct 15, 2020

Karsten1987 commented Oct 19, 2020

emersonknapp left a comment

Karsten1987 commented Oct 28, 2020

Karsten1987 commented Oct 28, 2020

adamdbrw commented Oct 29, 2020 •

edited

pijaro commented Oct 29, 2020

emersonknapp commented Oct 29, 2020

adamdbrw commented Nov 2, 2020 •

edited

Karsten1987 commented Nov 2, 2020

adamdbrw commented Nov 2, 2020

emersonknapp commented Nov 2, 2020 •

edited by adamdbrw

adamdbrw commented Nov 3, 2020 •

edited

Karsten1987 left a comment

Karsten1987 Nov 9, 2020

adamdbrw Nov 10, 2020 •

edited

pijaro commented Nov 10, 2020

adamdbrw commented Nov 10, 2020

Karsten1987 left a comment

adamdbrw commented Nov 12, 2020 •

edited

Karsten1987 commented Nov 13, 2020

		{"journal_mode", pragma_assign + "WAL"},
		{"synchronous", pragma_assign + "NORMAL"}

read yaml config file #497

read yaml config file #497

Conversation

Karsten1987 commented Aug 7, 2020

Karsten1987 commented Sep 1, 2020

emersonknapp left a comment

Choose a reason for hiding this comment

adamdbrw commented Oct 12, 2020

adamdbrw commented Oct 15, 2020

Karsten1987 commented Oct 19, 2020

emersonknapp left a comment

Choose a reason for hiding this comment

Karsten1987 commented Oct 28, 2020

Karsten1987 commented Oct 28, 2020

adamdbrw commented Oct 29, 2020 • edited

pijaro commented Oct 29, 2020

emersonknapp commented Oct 29, 2020

adamdbrw commented Nov 2, 2020 • edited

Karsten1987 commented Nov 2, 2020

adamdbrw commented Nov 2, 2020

emersonknapp commented Nov 2, 2020 • edited by adamdbrw

adamdbrw commented Nov 3, 2020 • edited

Karsten1987 left a comment

Choose a reason for hiding this comment

Karsten1987 Nov 9, 2020

Choose a reason for hiding this comment

adamdbrw Nov 10, 2020 • edited

Choose a reason for hiding this comment

pijaro commented Nov 10, 2020

adamdbrw commented Nov 10, 2020

Karsten1987 left a comment

Choose a reason for hiding this comment

adamdbrw commented Nov 12, 2020 • edited

Karsten1987 commented Nov 13, 2020

adamdbrw commented Oct 29, 2020 •

edited

adamdbrw commented Nov 2, 2020 •

edited

emersonknapp commented Nov 2, 2020 •

edited by adamdbrw

adamdbrw commented Nov 3, 2020 •

edited

adamdbrw Nov 10, 2020 •

edited

adamdbrw commented Nov 12, 2020 •

edited