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

[22537] Fix EDP reliability timings #5540

Merged
merged 3 commits into from
Jan 9, 2025
Merged

[22537] Fix EDP reliability timings #5540

merged 3 commits into from
Jan 9, 2025

Conversation

fujitatomoya
Copy link
Contributor

@fujitatomoya fujitatomoya commented Jan 7, 2025 •
edited by MiguelCompany
Loading

Description

happened to find this miscalculation during ros2/rmw_fastrtps#789.
i believe that comments are the correct values, so that default Duration_t adjusted accordingly in milliseconds.

@Mergifyio backport 3.1.x 3.0.x 2.14.x 2.10.x

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • N/A: Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • N/A: Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • N/A: New feature has been added to the versions.md file (if applicable).
  • N/A: New feature has been documented/Current behavior is correctly described in the documentation.
  • Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@fujitatomoya
Copy link
Contributor Author

@MiguelCompany can you take a look?

@MiguelCompany MiguelCompany self-requested a review January 7, 2025 07:12
@MiguelCompany MiguelCompany added this to the v3.2.0 milestone Jan 7, 2025
@MiguelCompany MiguelCompany changed the title EDP default configuration should be in milliseconds, not microseconds. Fix EDP reliability timings Jan 7, 2025
@fujitatomoya fujitatomoya force-pushed the edp-default-parameters-in-milliseconds branch from 2b6331a to d6fc341 Compare January 7, 2025 18:15
@fujitatomoya
Copy link
Contributor Author

@MiguelCompany took your suggestion, can you check and approve?

@MiguelCompany MiguelCompany self-requested a review January 8, 2025 06:47
MiguelCompany
MiguelCompany previously approved these changes Jan 8, 2025
View reviewed changes
@MiguelCompany MiguelCompany added the ci-pending PR which CI is running label Jan 8, 2025
@MiguelCompany
Copy link
Member

@Mergifyio backport 3.1.x 3.0.x 2.14.x 2.10.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 8, 2025 •
edited
Loading

backport 3.1.x 3.0.x 2.14.x 2.10.x

✅ Backports have been created

@MiguelCompany MiguelCompany changed the title Fix EDP reliability timings [22537] Fix EDP reliability timings Jan 8, 2025
@MiguelCompany
Copy link
Member

@fujitatomoya BlackboxTests_DDS_PIM.DDSStatistics.correct_deletion_upon_delete_contained_entities has failed both in Ubuntu and Windows, so it seems like a regression. Could you take a look?

@Mario-DL
Refs #22537: test fix
cf4d45b 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL Mario-DL self-requested a review January 9, 2025 08:40
@MiguelCompany MiguelCompany self-requested a review January 9, 2025 09:26
MiguelCompany
MiguelCompany approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@MiguelCompany MiguelCompany merged commit b53a389 into eProsima:master Jan 9, 2025
24 of 31 checks passed
mergify bot pushed a commit that referenced this pull request Jan 9, 2025
@fujitatomoya @mergify
Fix EDP reliability timings (#5540)
56c21f3 
* EDP default configuration should be in milliseconds, not microseconds.

Signed-off-by: Tomoya Fujita <[email protected]>

* add the timeout for writer->wait_discovery().

Signed-off-by: Tomoya Fujita <[email protected]>

* Refs #22537: test fix

Signed-off-by: Mario Dominguez <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Mario Dominguez <[email protected]>
Co-authored-by: Mario Dominguez <[email protected]>
(cherry picked from commit b53a389)
@mergify mergify bot mentioned this pull request Jan 9, 2025
Open
12 tasks
mergify bot pushed a commit that referenced this pull request Jan 9, 2025
@fujitatomoya @mergify
Fix EDP reliability timings (#5540)
b106e01 
* EDP default configuration should be in milliseconds, not microseconds.

Signed-off-by: Tomoya Fujita <[email protected]>

* add the timeout for writer->wait_discovery().

Signed-off-by: Tomoya Fujita <[email protected]>

* Refs #22537: test fix

Signed-off-by: Mario Dominguez <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Mario Dominguez <[email protected]>
Co-authored-by: Mario Dominguez <[email protected]>
(cherry picked from commit b53a389)
mergify bot pushed a commit that referenced this pull request Jan 9, 2025
@fujitatomoya @mergify
Fix EDP reliability timings (#5540)
9a2c44b 
* EDP default configuration should be in milliseconds, not microseconds.

Signed-off-by: Tomoya Fujita <[email protected]>

* add the timeout for writer->wait_discovery().

Signed-off-by: Tomoya Fujita <[email protected]>

* Refs #22537: test fix

Signed-off-by: Mario Dominguez <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Mario Dominguez <[email protected]>
Co-authored-by: Mario Dominguez <[email protected]>
(cherry picked from commit b53a389)

# Conflicts:
#	src/cpp/rtps/builtin/discovery/endpoint/EDPSimple.cpp
@mergify mergify bot mentioned this pull request Jan 9, 2025
Merged
12 tasks
@mergify mergify bot mentioned this pull request Jan 9, 2025
Open
12 tasks
mergify bot pushed a commit that referenced this pull request Jan 9, 2025
@fujitatomoya @mergify
Fix EDP reliability timings (#5540)
dac1d94 
* EDP default configuration should be in milliseconds, not microseconds.

Signed-off-by: Tomoya Fujita <[email protected]>

* add the timeout for writer->wait_discovery().

Signed-off-by: Tomoya Fujita <[email protected]>

* Refs #22537: test fix

Signed-off-by: Mario Dominguez <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: Mario Dominguez <[email protected]>
Co-authored-by: Mario Dominguez <[email protected]>
(cherry picked from commit b53a389)

# Conflicts:
#	src/cpp/rtps/builtin/discovery/endpoint/EDPSimple.cpp
@mergify mergify bot mentioned this pull request Jan 9, 2025
Open
12 tasks
@fujitatomoya
Copy link
Contributor Author

@Mario-DL thanks!

@fujitatomoya fujitatomoya mentioned this pull request Jan 13, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mario-DL Mario-DL Mario-DL approved these changes

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
ci-pending PR which CI is running
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fujitatomoya @MiguelCompany @Mario-DL