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

[21095] Example refactor: Discovery Server #4913

Merged
merged 18 commits into from
Jul 3, 2024
Merged

[21095] Example refactor: Discovery Server #4913

merged 18 commits into from
Jul 3, 2024

Conversation

Mario-DL
Copy link
Member

@Mario-DL Mario-DL commented Jun 7, 2024 •
edited by EduPonz
Loading

Description

This PR refactors the DiscoveryServerExample with the current new example format. It also adds proper tests for the examples testing automation.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • NO 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
  • NO Any new/modified methods have been properly documented using Doxygen.
  • NO Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • NO Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • NO Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • NO New feature has been documented/Current behavior is correctly described in the documentation.
  • NO 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.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@Mario-DL Mario-DL added this to the v3.0.0 milestone Jun 7, 2024
@Mario-DL Mario-DL force-pushed the feature/example_refactor/discovery-server branch from 02b9c6d to a251a3e Compare June 7, 2024 09:55
@github-actions github-actions bot added the ci-pending PR which CI is running label Jun 7, 2024
@Mario-DL Mario-DL force-pushed the feature/example_refactor/discovery-server branch from a251a3e to 58666d0 Compare June 7, 2024 10:31
@Mario-DL Mario-DL requested review from richiprosima and removed request for richiprosima June 7, 2024 10:31
@JesusPoderoso JesusPoderoso mentioned this pull request Jun 10, 2024
Closed
15 tasks
@Mario-DL
Copy link
Member Author

Discovery server tests passed although configuration timed out. @elianalf I guess we can start reviewing and later on I will rebase on top of the example splitting PR

@Mario-DL Mario-DL force-pushed the feature/example_refactor/discovery-server branch from 58666d0 to 8c07ead Compare June 10, 2024 13:14
@Mario-DL Mario-DL requested review from elianalf and removed request for richiprosima June 10, 2024 13:15
elianalf
elianalf suggested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@elianalf elianalf left a comment

Choose a reason for hiding this comment

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

What a great refactor!

Only one thing, I have tried to run the Server with transport UDPV6 and TCPV6 and it fails creating the participant, in particular because "Discovery Server wasn't able to allocate the specified listening port."

examples/cpp/discovery_server/main.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/main.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientSubscriberApp.cpp Show resolved Hide resolved
examples/cpp/discovery_server/ClientSubscriberApp.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientPublisherApp.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientPublisherApp.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientPublisherApp.cpp Outdated Show resolved Hide resolved
@Mario-DL
Copy link
Member Author

What a great refactor!

Only one thing, I have tried to run the Server with transport UDPV6 and TCPV6 and it fails creating the participant, in particular because "Discovery Server wasn't able to allocate the specified listening port."

Ty !
Its true, good catch, it was the fact that if the listening address is not specified in the server we need to change the default one to ::1

@Mario-DL Mario-DL requested review from elianalf and removed request for elianalf June 11, 2024 09:34
elianalf
elianalf suggested changes Jun 11, 2024
View reviewed changes
test/examples/test_discovery_server.py Outdated Show resolved Hide resolved
@Mario-DL Mario-DL requested a review from elianalf June 12, 2024 09:45
elianalf
elianalf suggested changes Jun 12, 2024
View reviewed changes
examples/cpp/discovery_server/CLIParser.hpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientPublisherApp.cpp Outdated Show resolved Hide resolved
examples/cpp/discovery_server/ClientPublisherApp.cpp Outdated Show resolved Hide resolved
@MiguelCompany
Copy link
Member

I think it would be better to rebase this on top of #4716, since we are changing the behavior of Discovery Server

@elianalf elianalf added no-test Skip CI tests if PR marked with this label and removed no-test Skip CI tests if PR marked with this label labels Jun 12, 2024
@elianalf elianalf self-requested a review June 12, 2024 14:09
elianalf
elianalf previously approved these changes Jun 12, 2024
View reviewed changes
@Mario-DL Mario-DL changed the base branch from master to feature/mesh_ds_guid_less_3.0.x June 18, 2024 07:53
@Mario-DL Mario-DL force-pushed the feature/example_refactor/discovery-server branch 2 times, most recently from 49ef7fe to f302422 Compare June 18, 2024 08:03
@cferreiragonz cferreiragonz force-pushed the feature/mesh_ds_guid_less_3.0.x branch from 09e9f9a to 688cd0f Compare June 28, 2024 06:45
Base automatically changed from feature/mesh_ds_guid_less_3.0.x to master July 3, 2024 06:16
@MiguelCompany MiguelCompany dismissed elianalf’s stale review July 3, 2024 06:16

The base branch was changed.

@Mario-DL
Refs #21095: Remove old DiscoveryServerExample
38f5168 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Add new discovery_server example
beb785d 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Add discovery_sever example tests
b96578b 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: versions.md
4f9ddfd 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Linter all except from types
d80b5b6 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: split discovery server example
f9595c0 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Address Elianas review
00ee139 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Linter
8da131a 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Fix casting for windows compilation
614eadc 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Create a test for each transport and intialize to ::1 wh…
8e2b38c 
…en ipv6 is used

Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Only change datawriter reliability when reliable is false
e6b3200 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Avoid using connection_ds option for the example. Althou…
3ed3a26 
…gh it is left for configurability

Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Add license
55ae6ab 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Remove -id- since it is no longer needed
273b821 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: Move namespace to fastdds
709ac36 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: regenerate types
b87274d 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL
Refs #21095: update headers
1c39079 
Signed-off-by: Mario Dominguez <[email protected]>
@Mario-DL Mario-DL force-pushed the feature/example_refactor/discovery-server branch from 4ccf71c to 1c39079 Compare July 3, 2024 07:36
@Mario-DL Mario-DL requested a review from elianalf July 3, 2024 07:37
@elianalf
Copy link
Contributor

elianalf commented Jul 3, 2024 •
edited
Loading

The only thing that needs to be changed is the guards in all the .hpp files, following the style in this PR, we can skip CI after the commit.

@Mario-DL
Refs #21095: Reword header marks
6e4f64b 
Signed-off-by: Mario Dominguez <[email protected]>
@elianalf
Copy link
Contributor

elianalf commented Jul 3, 2024

LGTM! CI passed here

@elianalf elianalf added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Jul 3, 2024
@EduPonz EduPonz removed the ci-pending PR which CI is running label Jul 3, 2024
@EduPonz EduPonz merged commit 16e0c0e into master Jul 3, 2024
2 of 3 checks passed
@EduPonz EduPonz deleted the feature/example_refactor/discovery-server branch July 3, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elianalf elianalf elianalf approved these changes

Assignees
No one assigned
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Mario-DL @MiguelCompany @elianalf @EduPonz