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.

Sign up for GitHub

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

Jump to bottom

Fix DATA_COUNT to account for number of destinations [11446] #1949

Merged
merged 5 commits into from
May 12, 2021
Merged

Fix DATA_COUNT to account for number of destinations [11446] #1949

merged 5 commits into from
May 12, 2021

Conversation

MiguelCompany
Copy link
Member

This PR builds on top of #1945 and fixes the way DATA_COUNT collect samples, in order to make it more accurate and also improve performance by calling the listener fewer times (once per send_any_unsent_changes / unsent_change_added_to_history call)

@MiguelCompany MiguelCompany changed the title Feature/statistics/rtps/data count rework Fix DATA_COUNT to account for number of destinations [11446] May 4, 2021
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/data-count-rework branch from 0de02a9 to 198e7fc Compare May 4, 2021 16:15
@MiguelCompany MiguelCompany temporarily deployed to codecov May 4, 2021 16:29 Inactive
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/data-count-rework branch 2 times, most recently from 6671995 to fbf9faf Compare May 7, 2021 11:07
@MiguelCompany MiguelCompany requested a review from MiguelBarro May 7, 2021 11:07
@MiguelCompany MiguelCompany temporarily deployed to codecov May 10, 2021 05:20 Inactive
MiguelBarro
MiguelBarro suggested changes May 10, 2021
View reviewed changes
Copy link
Contributor

@MiguelBarro MiguelBarro left a comment

Choose a reason for hiding this comment

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

On test RTPSStatisticsTests.statistics_rpts_avoid_empty_resent_callbacks the SAMPLE_DATAS callback is expected:

Fast-DDS/test/unittest/statistics/rtps/RTPSStatisticsTests.cpp

Lines 926 to 931 in 8def391

EXPECT_CALL(*writer_listener, on_heartbeat_count)
.Times(AtLeast(1));
EXPECT_CALL(*writer_listener, on_data_count)
.Times(AtLeast(1));
EXPECT_CALL(*writer_listener, on_sample_datas)
.Times(AtLeast(1));

@MiguelCompany
Refs 11446. Refactored StatisticsWriterImpl::on_data.
8883a27 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs 11446. Changes on RTPSWriter.
2f22e29 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs 11446. Sepparate on_data generated / sent.
099c346 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs 11446. Removed unused vector matched_readers_.
c0e7095 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs 11446. Fix after rebase.
75aebbf 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/data-count-rework branch from fbf9faf to 75aebbf Compare May 11, 2021 07:06
@MiguelCompany
Copy link
Member Author

On test RTPSStatisticsTests.statistics_rpts_avoid_empty_resent_callbacks the SAMPLE_DATAS callback is expected:

I have rebased this PR, as this has already been fixed on master.

@MiguelCompany MiguelCompany requested a review from MiguelBarro May 11, 2021 07:14
@MiguelCompany MiguelCompany temporarily deployed to codecov May 11, 2021 07:15 Inactive
MiguelBarro
MiguelBarro approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@MiguelBarro MiguelBarro left a comment

Choose a reason for hiding this comment

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

LGTM

@MiguelCompany MiguelCompany merged commit cec6e08 into master May 12, 2021
@MiguelCompany MiguelCompany deleted the feature/statistics/rtps/data-count-rework branch May 12, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelBarro MiguelBarro MiguelBarro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MiguelCompany @MiguelBarro