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

Collection of data and notification to listeners RTPS_PACKETS_LOST [10795] #1956

Merged
merged 20 commits into from
May 18, 2021

Conversation

MiguelCompany
Copy link
Member

No description provided.

@MiguelCompany MiguelCompany temporarily deployed to codecov May 13, 2021 05:51 Inactive
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/rtps-lost branch from 1c704de to a65676b Compare May 13, 2021 06:07
@MiguelCompany MiguelCompany temporarily deployed to codecov May 13, 2021 06:08 Inactive
MiguelBarro
MiguelBarro previously approved these changes May 17, 2021
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 so far

@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/rtps-lost branch from a65676b to cc8a2ff Compare May 17, 2021 14:43
@MiguelCompany MiguelCompany temporarily deployed to codecov May 17, 2021 14:51 Inactive
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/rtps-lost branch from cc8a2ff to 6afdc81 Compare May 18, 2021 06:16
@MiguelCompany MiguelCompany temporarily deployed to codecov May 18, 2021 06:22 Inactive
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/statistics/rtps/rtps-lost branch from 41ff744 to 36f1c16 Compare May 18, 2021 07:04
@MiguelCompany MiguelCompany temporarily deployed to codecov May 18, 2021 07:10 Inactive
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
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.

Please, correct the gtest fixture flaw you detected by replacing:

std::string str("https://github.com/eProsima/Fast-DDS.git");
memcpy(writer_change->serializedPayload.data, str.c_str(), str.length());
writer_change->serializedPayload.length = (uint32_t)str.length();

with

        std::string str("https://github.com/eProsima/Fast-DDS.git");
        writer_change->serializedPayload.length = std::min(length, static_cast<uint32_t>(str.length()));
        memcpy(writer_change->serializedPayload.data,
                str.c_str(),
                writer_change->serializedPayload.length);

Comment on lines 1362 to 1363
msg->length -= statistics_submessage_length;
msg->pos = msg->length;
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to prevent side effects is better practice to stash the msg state before the read_statistics_submessage call using local variables.
Note that read_statistics_submessage original implementation avoided side effects and stashing would have prevented applying the correction twice.

Entity2LocatorTraffic data{};
rtps::StatisticsSubmessageData::Sequence seq_data{};
};
std::map<lost_traffic_key, lost_traffic_value> lost_traffic;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the style guide traffic and lost_traffic should be underscored.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
MiguelBarro
MiguelBarro previously approved these changes May 18, 2021
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

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany temporarily deployed to codecov May 18, 2021 10:15 Inactive
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 cc7be2f into master May 18, 2021
@MiguelCompany MiguelCompany deleted the feature/statistics/rtps/rtps-lost branch May 18, 2021 12:40
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.

2 participants