Improve sqlite usage and test stability #31

anhosi · 2018-08-31T13:22:03Z

This pr is based on #30.

Improved handling of sqlite:

Open sqlite db according to io_flag, i.e. read-only or read-write. Only an existing db can be opened read-only.
Use journal mode WAL (write ahead logging) which improves concurrency and performance.
Use the multi-thread threading mode that allows concurrent db access without relying on mutexes to protect most db operations. Db handles may not be shared across threads but each thread must use its own handle.
Provides a convenient method for accessing the first row of a query result.

Other:

Fixed threading issues in tests
Allow direct linking against the sqlite storage plugin. This is needed by the integration tests so that assertions for the db content are possible. Also this enables reuse of the sqlite storage plugin in other plugins.

anhosi · 2018-08-31T13:38:20Z

CI:

Linux
Linux-aarch64
macOS
Windows

anhosi · 2018-08-31T14:44:16Z

New CI after fixing package dependencies:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-09-03T07:05:36Z

New CI after fixes:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-09-03T07:25:57Z

And another CI after fixing the typesupport assertion (names are apparently different on aarch, so let's just assert rosidl substring). The mac failure is a bit weird - any ideas?

Linux
Linux-aarch64
macOS
Windows

Also already links write integration test against the default plugins.

Query the underlying db directly in tests to determine the amount of recorded messages.

- Always open db with threading mode multi-thread. This forbids sharing database connections across threads. Db access from multiple threads is possible when each thread uses its own connection. - Open sqlite db accordingly to given io flags. Readonly open works only with already existing database. - Set journal mode pragma to WAL (write ahead log) and synchronous pragma to NORMAL. This should yield good write performance. - Small fix: use *.db3 as db name in tests.

Karsten1987

I think rosbag2 shouldn't have any dependencies (even if only test) to any specific storage format.
We could introduce a new package for testing in order to have to integration tested thoroughly.

Karsten1987 · 2018-09-11T17:57:24Z

rosbag2/test/rosbag2/rosbag2_test_fixture.hpp

@@ -42,7 +42,7 @@ class Rosbag2TestFixture : public Test
 {
 public:
  Rosbag2TestFixture()
-  : database_name_(UnitTest::GetInstance()->current_test_info()->name())
+  : database_name_(std::string(UnitTest::GetInstance()->current_test_info()->name()) + ".db3")


does this imply dependencies to sqlite storage? I feel like all tests within rosbag2 should be independent of any storage format.

Karsten1987 · 2018-09-11T18:01:20Z

rosbag2/test/rosbag2/rosbag2_write_integration_fixture.hpp

@@ -24,6 +24,8 @@
 #include "rclcpp/rclcpp.hpp"
 #include "rosbag2/rosbag2.hpp"
 #include "rosbag2_storage/serialized_bag_message.hpp"
+#include "rosbag2_storage_default_plugins/sqlite/sqlite_exception.hpp"


I think this implies a strong dependency from rosbag2 to rosbag2_storage_default_plugins. Ideally all these tests should perform equivally with all plugins.

We can introduce a new package for integration or system tests like these.

In order to write integration tests for rosbag2 we need a storage plugin. Which storage plugin is used for the tests is irrelevant and hidden inside the test fixtures.
Having rosbag2_storage_default_plugins as test dependency for rosbag2 or having a separate test package depending on rosbag2_storage_default_plugins and rosbag2 is for me practically equivalent. From a developers point of view I would prefer the test dependency as then the tests reside in the same package as the code they test.

When the rosbag2_transport package is introduced the tests will move there. If we choose to move the tests into a package of their own I suggest to do so after/ with the introduction of rosbag2_transport.

Karsten1987 · 2018-09-11T18:02:52Z

rosbag2/test/rosbag2/rosbag2_write_integration_fixture.hpp

            std::this_thread::sleep_for(50ms);
          }
        }
    ));
  }

+  size_t count_stored_messages(rosbag2_storage_plugins::SqliteWrapper & db, std::string topic_name)


const std::string &

Karsten1987 · 2018-09-11T18:04:35Z

rosbag2/test/rosbag2/rosbag2_write_integration_fixture.hpp

+      try {
+        return count_stored_messages_in_db(db, topic_name);
+      } catch (const rosbag2_storage_plugins::SqliteException & e) {
+        (void) e;


shouldn't this break the while loop? Is there a way to have the loop conditioned otherwise than while (true)? This seems just dangerous.

The test runner already provides a timeout so the while (true) while never run forever.
If a query fails to lock the sqlite db the wrapper throws (see also comment below). This loop simply retries the query until it succeeds (or the test wrapper kills and fails the test). It could be rewritten but this would only hide the intention of looping until the query succeeds.

Karsten1987 · 2018-09-11T18:05:26Z

rosbag2/test/rosbag2/rosbag2_write_integration_fixture.hpp

            std::this_thread::sleep_for(50ms);
          }
        }
    ));
  }

+  size_t count_stored_messages(rosbag2_storage_plugins::SqliteWrapper & db, std::string topic_name)
+  {
+    // protect against concurrent writes (from recording) that may make the count query throw.


if called concurrently, shouldn't this be locked then?

We decided no for the following reason: the rosbag2_storage_plugins::SqliteWrapper is not designed to handle db locking more gracefully than throwing an exception as for the "record" and "play" usecases there will be no concurrent usage of the SqliteWrapper. Even for a later "convert" usage concurrent usage of the same db is (almost certain) not needed. So the concurrent use is only needed in tests the handling of db locks is put in the test fixture.
Retry is the most common strategy for dealing with failures of obtaining a db lock. Every other strategy would yield much more complicated code and the test runners timeout protects against infinite retrying.

Karsten1987 · 2018-09-11T18:08:22Z

..._storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_exception.hpp

+
+#ifdef _WIN32
+# pragma warning(push)
+# pragma warning(disable:4275)


what's that warning? Can you maybe put a little comment on this to trace it later on?

Karsten1987 · 2018-09-11T18:08:56Z

..._default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp

+
+#ifdef _WIN32
+# pragma warning(push)
+# pragma warning(disable:4251)


same as above. Comment could be helpful here.

Karsten1987 · 2018-09-11T18:09:21Z

..._default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp


 namespace rosbag2_storage_plugins
 {

-class SqliteStatementWrapper : public std::enable_shared_from_this<SqliteStatementWrapper>
+class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStatementWrapper
+  : public std::enable_shared_from_this<SqliteStatementWrapper>
 {
 public:
  SqliteStatementWrapper(sqlite3 * database, std::string query);


Karsten1987 · 2018-09-11T18:10:53Z

...rage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp

@@ -51,8 +51,9 @@ SqliteStatementWrapper::~SqliteStatementWrapper()
 std::shared_ptr<SqliteStatementWrapper> SqliteStatementWrapper::execute_and_reset()
 {
  int return_code = sqlite3_step(statement_);
-  if (return_code != SQLITE_OK && return_code != SQLITE_DONE) {
-    throw SqliteException("Error processing SQLite statement.");
+  if (return_code != SQLITE_OK && return_code != SQLITE_DONE && return_code != SQLITE_ROW) {


nitpick: Maybe for readability, these checks can be excluded in a little inline function is_valid or something. So ease up the condition.

Karsten1987 · 2018-09-11T18:11:32Z

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

@@ -25,7 +25,8 @@

 #include "rosbag2_storage/serialized_bag_message.hpp"
 #include "../logging.hpp"
-#include "sqlite_statement_wrapper.hpp"
+#include "rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.hpp"


nit: alphabetical order with ../logging.hpp

- consistently use const ref of string instead of string for function arguments - simplify package dependencies - minor formatting

botteroa-si · 2018-09-13T13:25:32Z

New CI after fixing failing test on Mac:

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2018-09-13T18:36:19Z

...rage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp

@@ -51,13 +51,18 @@ SqliteStatementWrapper::~SqliteStatementWrapper()
 std::shared_ptr<SqliteStatementWrapper> SqliteStatementWrapper::execute_and_reset()
 {
  int return_code = sqlite3_step(statement_);
-  if (return_code != SQLITE_OK && return_code != SQLITE_DONE && return_code != SQLITE_ROW) {
+  if (!isQueryOk(return_code)) {


nitpick: ros2 convention is lower case and underscore

Karsten1987 · 2018-09-13T18:36:47Z

...rage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp

    throw SqliteException("Error processing SQLite statement. Return code: " +
            std::to_string(return_code));
  }
  return reset();
 }

+bool SqliteStatementWrapper::isQueryOk(int return_code)


could be inline or set in a lamdba

* remove unnecessary block Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * use target_link_libraries instead of ament_target_dependencies Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * remove ros environment Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * add prefix to compile definition Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

* remove unnecessary block Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * use target_link_libraries instead of ament_target_dependencies Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * remove ros environment Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> * add prefix to compile definition Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> Signed-off-by: James Smith <james@foxglove.dev>

tfoote added the in review label Aug 31, 2018

Martin-Idel-SI mentioned this pull request Aug 31, 2018

rosbag_write_integration_test is flaky #25

Closed

anhosi force-pushed the feature/replace_after_write_action branch 2 times, most recently from 8e7e2bd to be2f5a2 Compare August 31, 2018 14:42

Martin-Idel-SI mentioned this pull request Sep 4, 2018

Allow an arbitrary topic to be recorded #26

Merged

Martin-Idel-SI force-pushed the feature/replace_after_write_action branch from d6105bd to 9da009e Compare September 4, 2018 08:34

anhosi mentioned this pull request Sep 5, 2018

Record and play multiple topics #27

Merged

anhosi force-pushed the feature/replace_after_write_action branch from 9da009e to bbe6171 Compare September 5, 2018 17:20

anhosi and others added 11 commits September 11, 2018 18:19

ros2GH-64 Rearrange default plugins build to use public headers

b674f92

Also already links write integration test against the default plugins.

ros2GH-64 Remove after_write_action

7e4bf1e

Query the underlying db directly in tests to determine the amount of recorded messages.

ros2GH-64 Add convenience getter for single line SQL result

078b860

ros2GH-64 Add visibility macros to enable linking on Windows

b9e25e4

ros2GH-64 Remove second sqlite exception class (it is not needed)

b828bac

ros2GH-64 Always log sqlite return code

e3ed85e

ros2GH-64 Fix hanging rosbag2_read_integration_test

822266e

ros2GH-64 Fix package test dependencies

5125d9a

ros2GH-64 Fix cppcheck error

586cefe

ros2GH-64 Fix asserting typesupport in test (varies on architectures)

0621595

Martin-Idel-SI force-pushed the feature/replace_after_write_action branch from bbe6171 to 0621595 Compare September 11, 2018 16:27

Karsten1987 requested changes Sep 11, 2018

View reviewed changes

anhosi and others added 3 commits September 11, 2018 23:51

Cleanup

0aa99e3

- consistently use const ref of string instead of string for function arguments - simplify package dependencies - minor formatting

Make play integration test compile on Mac

d6d9c75

Fix sqlite_wrapper_integration_test

2477321

Karsten1987 approved these changes Sep 13, 2018

View reviewed changes

Karsten1987 merged commit 4bfa3c6 into ros2:master Sep 13, 2018

Karsten1987 removed the in review label Sep 13, 2018

Martin-Idel-SI deleted the feature/replace_after_write_action branch September 14, 2018 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sqlite usage and test stability #31

Improve sqlite usage and test stability #31

anhosi commented Aug 31, 2018

anhosi commented Aug 31, 2018

anhosi commented Aug 31, 2018

Martin-Idel-SI commented Sep 3, 2018

Martin-Idel-SI commented Sep 3, 2018 •

edited

Loading

Karsten1987 left a comment

Karsten1987 Sep 11, 2018

Karsten1987 Sep 11, 2018

anhosi Sep 11, 2018

Karsten1987 Sep 11, 2018

Karsten1987 Sep 11, 2018

anhosi Sep 11, 2018

Karsten1987 Sep 11, 2018

anhosi Sep 11, 2018

Karsten1987 Sep 11, 2018

Karsten1987 Sep 11, 2018

Karsten1987 Sep 11, 2018

Karsten1987 Sep 11, 2018

anhosi Sep 11, 2018

Karsten1987 Sep 11, 2018

botteroa-si commented Sep 13, 2018

Karsten1987 Sep 13, 2018

Karsten1987 Sep 13, 2018

Improve sqlite usage and test stability #31

Improve sqlite usage and test stability #31

Conversation

anhosi commented Aug 31, 2018

anhosi commented Aug 31, 2018

anhosi commented Aug 31, 2018

Martin-Idel-SI commented Sep 3, 2018

Martin-Idel-SI commented Sep 3, 2018 • edited Loading

Karsten1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botteroa-si commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Sep 3, 2018 •

edited

Loading