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

[20334] Make the resource limits consistent after changing states #140

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

Tempate
Copy link
Contributor

@Tempate Tempate commented Apr 18, 2024

In the previous version, the resource limits only applied to the DDS Recorder while it was running: as soon as the DDS Recorder received a STOP, the resource limits were reset. In this version, the DDS Recorder can rotate the files from a previous running session. Moreover, in this version users can also set AVOID_OVERWRITING_FILES when stopping to save a set of files from the DDS Recorder rotations.

This version also fixes some bugs.

Merge after:

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 43.62934% with 292 lines in your changes missing coverage. Please review.

Project coverage is 23.74%. Comparing base (59df593) to head (674f0c2).

Files Patch % Lines
...participants/src/cpp/recorder/mcap/McapHandler.cpp 23.02% 39 Missing and 68 partials ⚠️
..._participants/src/cpp/recorder/mcap/McapWriter.cpp 53.14% 21 Missing and 61 partials ⚠️
...rticipants/src/cpp/recorder/output/FileTracker.cpp 47.42% 26 Missing and 25 partials ⚠️
...icipants/src/cpp/recorder/mcap/McapSizeTracker.cpp 33.33% 16 Missing and 10 partials ⚠️
ddsrecorder/src/cpp/tool/DdsRecorder.cpp 44.00% 5 Missing and 9 partials ⚠️
ddsrecorder/src/cpp/main.cpp 0.00% 5 Missing ⚠️
...der_participants/recorder/mcap/impl/McapWriter.ipp 50.00% 0 Missing and 5 partials ⚠️
...participants/src/cpp/recorder/mcap/McapMessage.cpp 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   22.61%   23.74%   +1.12%     
==========================================
  Files          46       55       +9     
  Lines        3895     4005     +110     
  Branches     2048     2070      +22     
==========================================
+ Hits          881      951      +70     
- Misses       2333     2370      +37     
- Partials      681      684       +3     

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

Tempate added 15 commits May 17, 2024 08:54
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
@Tempate Tempate changed the title Make the resource limits consistent after changing states [20334] Make the resource limits consistent after changing states Jun 5, 2024
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
@@ -41,7 +41,9 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include ddspipe_core/types/dds/Payload.hpp

DDSRECORDER_MCAP_HANDLER,
"Ignoring stop command, instance already stopped.");
logWarning(DDSRECORDER_MCAP_HANDLER,
"MCAP_STATE | Ignoring stop command, instance already stopped.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this name, it's not giving information about the MCAP state but about the MCAPHANDLER

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is the McapHandler is a concept that comes from how the code works. A user shouldn't be aware of what the McapHandler is or what it's in charge of. It should just know high level concepts, like state and write.

Signed-off-by: tempate <danieldiaz@eprosima.com>
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.

None yet

4 participants