[21413] Record data in SQL #173

EugenioCollado · 2024-12-19T09:46:40Z

Code improvements:

Make a Serializer to serialize and deserialize TopicQoS, TypeObjects, TypeIdentifiers, and DynamicTypes.

Make a time_utils to convert between different time formats.

Make the DdsRecorder and the DdsReplayer agnostic of the output format.

Refactor McapLogErrorTest to improve its readability.

Configuration improvements:

Move cleanup-period and max-pending-samples from specs to recorder.

Configure safety-margin as a string (100MB, 128GiB, etc).

Bug fixes:

Avoid creating a payload when replaying a message in a topic without a reader.

```
Fix assert in ROS 2 topics.
```

Potential TODOs:

Separate data storage calls to dedicated thread

codecov-commenter · 2024-12-19T09:55:46Z

Codecov Report

Attention: Patch coverage is 40.29944% with 957 lines in your changes missing coverage. Please review.

Project coverage is 29.33%. Comparing base (37d1303) to head (a3ce840).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...er_participants/src/cpp/recorder/sql/SqlWriter.cpp	31.21%	103 Missing and 113 partials ⚠️
...rticipants/src/cpp/recorder/output/BaseHandler.cpp	42.85%	74 Missing and 70 partials ⚠️
...icipants/src/cpp/replayer/SqlReaderParticipant.cpp	26.85%	24 Missing and 55 partials ⚠️
ddsrecorder/src/cpp/tool/DdsRecorder.cpp	44.68%	51 Missing and 27 partials ⚠️
..._yaml/src/cpp/recorder/YamlReaderConfiguration.cpp	26.53%	27 Missing and 45 partials ⚠️
...cipants/src/cpp/replayer/McapReaderParticipant.cpp	27.63%	9 Missing and 46 partials ⚠️
...r_participants/src/cpp/recorder/sql/SqlHandler.cpp	38.80%	8 Missing and 33 partials ⚠️
...participants/src/cpp/recorder/mcap/McapHandler.cpp	34.21%	6 Missing and 19 partials ⚠️
...rticipants/src/cpp/recorder/message/SqlMessage.cpp	41.86%	7 Missing and 18 partials ⚠️
...rticipants/src/cpp/common/serialize/Serializer.cpp	56.60%	2 Missing and 21 partials ⚠️
... and 18 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   25.17%   29.33%   +4.15%     
==========================================
  Files          75       70       -5     
  Lines        5842     4445    -1397     
  Branches     2999     2262     -737     
==========================================
- Hits         1471     1304     -167     
+ Misses       3408     2144    -1264     
- Partials      963      997      +34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

juanlofer-eprosima · 2025-01-28T13:53:54Z

ddsrecorder/CMakeLists.txt

@@ -16,8 +16,10 @@
 # CMake build rules for DDS Recorder Submodule
 ###############################################################################
 cmake_minimum_required(VERSION 3.5)
+set(CMAKE_SYSTEM_VERSION 10.0)


I am not sure this is the best approach. First, apparently this only makes sense in Windows, so it should be within an if. I also wonder if we should be imposing the sdk version here or we should investigate which versions are compatible with sqlite and warn the user she should be compiling with one of those (and specify it via cmake args). Otherwise, we could be forcing a version not available in a user's machine when another compatible one is present.

juanlofer-eprosima · 2025-01-28T13:56:45Z

ddsrecorder/CMakeLists.txt


 # Done this to set machine architecture and be able to call cmake_utils
+enable_language(C)


Maybe this, and the new statement above should only be added to blackbox tests using sqlite.

juanlofer-eprosima · 2025-01-28T14:05:52Z

ddsrecorder/src/cpp/main.cpp

+                if(!config_loaded)
+                {
+                    configuration = eprosima::ddsrecorder::yaml::RecorderConfiguration(commandline_args.file_path);
+                }
+                else
+                {
+                    config_loaded = false;
+                }


I am not sure this is an improvement. I agree it would be better to avoid two immediate consecutive configuration loads, as it is the case when the initial state is running or paused. But a long time might have passed when coming from a stopped state, and the user might have changed the allowlist in the meantime.

juanlofer-eprosima · 2025-01-28T15:17:04Z

ddsrecorder/src/cpp/tool/DdsRecorder.hpp

@@ -47,8 +46,8 @@ ENUMERATION_BUILDER(
    DdsRecorderStateCode,
    STOPPED,                  //! Internal entities are not created and thus no messages are received.
    SUSPENDED,                //! Messages are received (internal entities created) but discarded.
-    RUNNING,                  //! Messages are stored in MCAP file.
-    PAUSED                    //! Messages are stored in buffer and stored in MCAP file if event triggered.
+    RUNNING,                  //! Messages are stored in MCAP/SQL file.


NIT: SQL file? or database?

juanlofer-eprosima · 2025-01-29T14:14:36Z

ddsrecorder/src/cpp/tool/DdsRecorder.hpp

@@ -81,15 +82,16 @@ class DdsRecorder
     *
     * @param configuration: Structure encapsulating all recorder configuration options.
     * @param init_state:    Initial instance state (RUNNING/PAUSED/SUSPENDED/STOPPED).
-     * @param event_handler: Reference to event handler used for thread synchronization in main application.
-     * @param file_tracker:  Reference to file tracker used to manage mcap files.
+     * @param mcap_file_tracker:  Reference to file tracker used to manage mcap files.


Please restore event_handler.

juanlofer-eprosima · 2025-02-06T15:19:10Z

ddsrecorder_participants/src/cpp/recorder/message/McapMessage.cpp

+    publishTime = to_mcap_timestamp(publish_time);
+    logTime = to_mcap_timestamp(log_time);
+
+    if (log_publish_time)
+    {
+        log_time = publish_time;
+    }


Suggested change

publishTime = to_mcap_timestamp(publish_time);

logTime = to_mcap_timestamp(log_time);

if (log_publish_time)

{

log_time = publish_time;

}

publishTime = to_mcap_timestamp(publish_time);

if (log_publish_time)

{

logTime = publishTime;

}

else

{

logTime = to_mcap_timestamp(log_time);

}

juanlofer-eprosima · 2025-02-06T15:34:41Z

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp


-        pending_samples.pop_front();
-    }
+    process_new_sample_nts_(std::make_shared<const McapMessage>(


I don't like the fact that you create the message for dropping it later on if state is stopped.

juanlofer-eprosima · 2025-02-06T15:41:31Z

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp

-                }
-                else
-                {
-                    // No schema available + no pending samples -> Add to buffer with blank schema


I personally prefer to have the explicit comment to make clear that doing nothing is the intended purpose.

juanlofer-eprosima · 2025-02-07T11:07:19Z

ddsrecorder_participants/src/cpp/recorder/output/BaseHandler.cpp

+    // NOTE: the outdated pending samples are not removed since they must be written as soon as they receive their type.
+
+    // Buffer
+    samples_buffer_.remove_if([&](const auto sample)


Why pass by value now? Avoid copy.

juanlofer-eprosima · 2025-02-07T11:36:06Z

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp

+    if (pending_samples_.find(type_name) != pending_samples_.end() ||
+            (state_ == BaseHandlerStateCode::PAUSED &&
+            pending_samples_paused_.find(type_name) != pending_samples_paused_.end()))
    {
-        add_pending_samples_nts_(type_name);
+        dump_pending_samples_nts_(type_name);
    }


Could you please encapsulate this in a parent method?

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

EugenioCollado temporarily deployed to codecov December 19, 2024 09:46 — with GitHub Actions Inactive

EugenioCollado requested a review from juanlofer-eprosima December 19, 2024 09:46

EugenioCollado temporarily deployed to codecov December 30, 2024 11:36 — with GitHub Actions Inactive

EugenioCollado force-pushed the feature/sql-3.x branch from d43dff9 to 1cee82a Compare December 30, 2024 14:30

EugenioCollado had a problem deploying to codecov December 30, 2024 14:30 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from 1cee82a to 7b12ed7 Compare December 30, 2024 14:39

EugenioCollado temporarily deployed to codecov December 30, 2024 14:39 — with GitHub Actions Inactive

EugenioCollado had a problem deploying to codecov December 30, 2024 15:04 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from 9a05ffd to 85807da Compare December 30, 2024 15:19

EugenioCollado temporarily deployed to codecov December 30, 2024 15:19 — with GitHub Actions Inactive

EugenioCollado temporarily deployed to codecov December 31, 2024 08:56 — with GitHub Actions Inactive

EugenioCollado force-pushed the feature/sql-3.x branch from 7a10ed7 to c1ec0ab Compare December 31, 2024 09:28

EugenioCollado temporarily deployed to codecov December 31, 2024 09:28 — with GitHub Actions Inactive

EugenioCollado force-pushed the feature/sql-3.x branch from c1ec0ab to d27b19c Compare December 31, 2024 14:19

EugenioCollado temporarily deployed to codecov December 31, 2024 14:19 — with GitHub Actions Inactive

EugenioCollado requested a review from richiprosima December 31, 2024 14:35

EugenioCollado force-pushed the feature/sql-3.x branch from d27b19c to 057bb4a Compare January 7, 2025 14:51

EugenioCollado temporarily deployed to codecov January 7, 2025 14:51 — with GitHub Actions Inactive

EugenioCollado had a problem deploying to codecov January 9, 2025 14:49 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from 3fad8fa to 52f80f4 Compare January 9, 2025 15:33

EugenioCollado had a problem deploying to codecov January 9, 2025 15:33 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from 52f80f4 to fc38df2 Compare January 13, 2025 08:28

EugenioCollado had a problem deploying to codecov January 13, 2025 08:29 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from fc38df2 to e6572a8 Compare January 13, 2025 08:40

EugenioCollado had a problem deploying to codecov January 13, 2025 08:40 — with GitHub Actions Failure

EugenioCollado force-pushed the feature/sql-3.x branch from e6572a8 to c285f07 Compare January 13, 2025 08:49

EugenioCollado had a problem deploying to codecov January 13, 2025 08:49 — with GitHub Actions Failure

EugenioCollado had a problem deploying to codecov January 13, 2025 15:01 — with GitHub Actions Failure

EugenioCollado had a problem deploying to codecov January 16, 2025 07:06 — with GitHub Actions Failure

EugenioCollado added 7 commits January 28, 2025 12:49

Documentation update

e01c8d7

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

Fix test sql log-file

cb0f53d

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

sqlite3 library amalgamation

6326c16

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

Fix data_cdr memory leak

006d3ce

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

ASAN fix

6ba899f

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

Windows fix

3014769

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

Force Windows SDK version

bd5271a

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

juanlofer-eprosima force-pushed the feature/sql-3.x branch from e2f98d6 to 70bce7a Compare January 28, 2025 12:01

juanlofer-eprosima temporarily deployed to codecov January 28, 2025 12:01 — with GitHub Actions Inactive

juanlofer-eprosima force-pushed the feature/sql-3.x branch from 70bce7a to 8d2f52a Compare January 28, 2025 13:27

juanlofer-eprosima temporarily deployed to codecov January 28, 2025 13:27 — with GitHub Actions Inactive

juanlofer-eprosima force-pushed the feature/sql-3.x branch from 8d2f52a to 74d9110 Compare January 28, 2025 13:52

juanlofer-eprosima temporarily deployed to codecov January 28, 2025 13:52 — with GitHub Actions Inactive

Fix Windows compilation

d74fe42

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>

juanlofer-eprosima force-pushed the feature/sql-3.x branch from 74d9110 to d74fe42 Compare January 28, 2025 15:07

juanlofer-eprosima temporarily deployed to codecov January 28, 2025 15:07 — with GitHub Actions Inactive

ASAN ignore ODR sqlite recorder_tool tests

c36fd7f

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

EugenioCollado temporarily deployed to codecov February 5, 2025 09:11 — with GitHub Actions Inactive

juanlofer-eprosima requested changes Feb 7, 2025

View reviewed changes

EugenioCollado temporarily deployed to codecov February 10, 2025 13:27 — with GitHub Actions Inactive

EugenioCollado had a problem deploying to codecov February 11, 2025 15:55 — with GitHub Actions Failure

EugenioCollado temporarily deployed to codecov February 12, 2025 13:22 — with GitHub Actions Inactive

Fix windows tests

a462998

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

EugenioCollado force-pushed the feature/sql-3.x branch from 3c78a92 to a462998 Compare February 12, 2025 13:41

EugenioCollado had a problem deploying to codecov February 12, 2025 13:41 — with GitHub Actions Error

EugenioCollado temporarily deployed to codecov February 12, 2025 13:58 — with GitHub Actions Inactive

Fix TSAN

a3ce840

Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>

EugenioCollado temporarily deployed to codecov February 13, 2025 13:21 — with GitHub Actions Inactive

EugenioCollado had a problem deploying to codecov February 13, 2025 13:21 — with GitHub Actions Failure

EugenioCollado had a problem deploying to codecov February 13, 2025 15:34 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[21413] Record data in SQL #173

[21413] Record data in SQL #173

EugenioCollado commented Dec 19, 2024

codecov-commenter commented Dec 19, 2024 •

edited

Loading

juanlofer-eprosima Jan 28, 2025

juanlofer-eprosima Jan 28, 2025

juanlofer-eprosima Jan 28, 2025

juanlofer-eprosima Jan 28, 2025

juanlofer-eprosima Jan 29, 2025

juanlofer-eprosima Feb 6, 2025

juanlofer-eprosima Feb 6, 2025

juanlofer-eprosima Feb 6, 2025

juanlofer-eprosima Feb 7, 2025

juanlofer-eprosima Feb 7, 2025


		# Done this to set machine architecture and be able to call cmake_utils
		enable_language(C)

[21413] Record data in SQL #173

Are you sure you want to change the base?

[21413] Record data in SQL #173

Conversation

EugenioCollado commented Dec 19, 2024

codecov-commenter commented Dec 19, 2024 • edited Loading

Codecov Report

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

codecov-commenter commented Dec 19, 2024 •

edited

Loading