[23974] Fix DataReader history enforcement to respect max_samples_per_instance#6228
[23974] Fix DataReader history enforcement to respect max_samples_per_instance#6228MiguelCompany merged 12 commits into3.4.xfrom
Conversation
🧪 CI InsightsHere's what we observed from your CI run for 5b0f1ee. ❌ Job Failures
|
37561c7 to
5259203
Compare
5259203 to
d7266a4
Compare
MiguelCompany
left a comment
There was a problem hiding this comment.
Apart from the comment below, remember to clarify the documentation (i.e. do a partial backport of eProsima/Fast-DDS-docs#1188)
Signed-off-by: Raül <raulojeda@eprosima.com>
… and add tests accordingly Signed-off-by: Raül <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
348cd88 to
0b4a87c
Compare
Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
…/Fast-DDS into hotfix/depth_vs_max_spi
Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com>
Signed-off-by: Raül <raulojeda@eprosima.com>
…/Fast-DDS into hotfix/depth_vs_max_spi
|
@Mergifyio backport 3.4.x 3.2.x 2.14.x |
✅ Backports have been createdDetails
No pull request needed: No commits between 3.4.x and mergify/bp/3.4.x/pr-6228
Cherry-pick of 30b6351 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 30b6351 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
#6228) * Fix DataReader history enforcement to respect max_samples_per_instance Signed-off-by: Raül <raulojeda@eprosima.com> * Take into account LENGTH_UNLIMITED, go back to max_samples allocation and add tests accordingly Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * More uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Refactor history depth vs mspi tests Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * Refactor tests and add keyed keep_all tests Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * New uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> --------- Signed-off-by: Raül <raulojeda@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 30b6351) # Conflicts: # test/unittest/dds/subscriber/DataReaderTests.cpp
#6228) * Fix DataReader history enforcement to respect max_samples_per_instance Signed-off-by: Raül <raulojeda@eprosima.com> * Take into account LENGTH_UNLIMITED, go back to max_samples allocation and add tests accordingly Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * More uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Refactor history depth vs mspi tests Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * Refactor tests and add keyed keep_all tests Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * New uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> --------- Signed-off-by: Raül <raulojeda@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 30b6351) # Conflicts: # src/cpp/fastdds/publisher/DataWriterHistory.cpp # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # test/mock/rtps/PublisherHistory/fastdds/publisher/DataWriterHistory.hpp # test/unittest/dds/subscriber/DataReaderTests.cpp
#6228) (#6285) * Fix DataReader history enforcement to respect max_samples_per_instance (#6228) * Fix DataReader history enforcement to respect max_samples_per_instance Signed-off-by: Raül <raulojeda@eprosima.com> * Take into account LENGTH_UNLIMITED, go back to max_samples allocation and add tests accordingly Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * More uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Refactor history depth vs mspi tests Signed-off-by: Raül <raulojeda@eprosima.com> * Uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * Refactor tests and add keyed keep_all tests Signed-off-by: Raül <raulojeda@eprosima.com> * Update src/cpp/fastdds/publisher/DataWriterHistory.cpp Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> * New uncrustify Signed-off-by: Raül <raulojeda@eprosima.com> --------- Signed-off-by: Raül <raulojeda@eprosima.com> Signed-off-by: Raül Ojeda Gandia <raulojeda@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 30b6351) # Conflicts: # src/cpp/fastdds/publisher/DataWriterHistory.cpp # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # test/mock/rtps/PublisherHistory/fastdds/publisher/DataWriterHistory.hpp # test/unittest/dds/subscriber/DataReaderTests.cpp * Fix conflicts and compilation issues Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Fix wrong backports Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Uncrustify Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Fix failing test Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Refactor pool config in DataWriterImpl Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Uncrustify Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> --------- Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> Co-authored-by: Raül Ojeda Gandia <raulojeda@eprosima.com> Co-authored-by: Emilio Cuesta <emiliocuesta@eprosima.com>
Description
This PR fixes a bug where
DataReaderwas incorrectly usinghistory.depthinstead ofmax_samples_per_instancewhendepth > max_samples_per_instance.Current Behavior (Bug)
When a DataReader is configured with:
history.depth = 10max_samples_per_instance = 5The QoS validation correctly warns:
However, the DataReader was actually storing 10 samples (using
depth) instead of 5 (usingmax_samples_per_instance), making the warning misleading.Root Cause
The bug existed in three locations:
max_samples_per_instancewas being overwritten withmax_samples, destroying the user's QoS settinginstance_changes.size() < depthinstead ofinstance_changes.size() < min(depth, max_samples_per_instance)Changes
min(history.depth, max_samples_per_instance)as the effective limitDataWriterHistory::to_history_attributesfor consistency in memory allocationImpact
This fix ensures the DataReader behavior matches the documented QoS warning message. Applications relying on
max_samples_per_instanceto limit memory usage will now work correctly.@Mergifyio backport 3.4.x 3.2.x 2.14.x
Contributor Checklist
versions.mdfile (if applicable).Reviewer Checklist