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

Discovery Server fix reconnection [12522] #2246

Merged
merged 7 commits into from
Nov 22, 2021
Merged

Conversation

jparisu
Copy link
Contributor

@jparisu jparisu commented Oct 4, 2021

Merge after #2244

@jparisu jparisu marked this pull request as ready for review October 5, 2021 09:02
@jparisu jparisu force-pushed the bugfix/ds-reconnection branch 2 times, most recently from 9879c57 to a035052 Compare October 6, 2021 09:16
@jparisu jparisu changed the title Discovery Server fix reconnection Discovery Server fix reconnection [12522] Oct 7, 2021
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.

Just a small nit.

@MiguelCompany
Copy link
Member

@jparisu Also please fix linters

@MiguelCompany MiguelCompany added this to the v2.4.1 milestone Nov 2, 2021
@jparisu
Copy link
Contributor Author

jparisu commented Nov 2, 2021

Test with discovery server PR eProsima/Discovery-Server#45

jparisu added 4 commits November 2, 2021 12:43
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
@jparisu jparisu force-pushed the bugfix/ds-reconnection branch from c779aee to 33a309e Compare November 2, 2021 11:43
jparisu added 2 commits November 2, 2021 12:45
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
MiguelCompany
MiguelCompany previously approved these changes Nov 2, 2021
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.

Code looks good to me, but I still have to verify with the new tests.

@MiguelCompany
Copy link
Member

MiguelCompany commented Nov 3, 2021

Discovery Server CI with new timeouts:
Build Status

@jparisu
Copy link
Contributor Author

jparisu commented Nov 3, 2021

It seems like some tests are failing due to time limits, because executing (with SHM and without SHM) one of them fails and the other don't.
Let me check in my local and change the times for a wider timeout.

@jparisu
Copy link
Contributor Author

jparisu commented Nov 3, 2021

The time has been slightly increased, it should be enough.
Please @MiguelCompany could you launch the tests again?

@MiguelCompany
Copy link
Member

Discovery Server CI with new timeouts:
Build Status

@MiguelCompany
Copy link
Member

Discovery Server CI with new timeouts: Build Status

This is just failing setting a PR status, as it was not launched from a PR.

@MiguelCompany MiguelCompany added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Nov 3, 2021
@EduPonz
Copy link

EduPonz commented Nov 11, 2021

@richiprosima please test this

@EduPonz
Copy link

EduPonz commented Nov 11, 2021

CI with corresponding Discovery Server tests:

Build Status

@@ -1755,25 +1755,20 @@ void DiscoveryDataBase::unmatch_participant_(
"Attempting to unmatch an unexisting participant: " << guid_prefix);
}
Copy link

Choose a reason for hiding this comment

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

I think this find is not necessary

Signed-off-by: jparisu <javierparis@eprosima.com>
Copy link

@EduPonz EduPonz left a comment

Choose a reason for hiding this comment

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

LGTM, I'll run a CI just in case

@EduPonz
Copy link

EduPonz commented Nov 15, 2021

Discovery Server tests with eProsima/Discovery-Server#45:

Build Status

@EduPonz
Copy link

EduPonz commented Nov 16, 2021

Build Status

I'm concerned about the test_39_trivial_reconnect.SHM_ON failure. What do you think @jparisu?

@jparisu
Copy link
Contributor Author

jparisu commented Nov 16, 2021

Build Status

I'm concerned about the test_39_trivial_reconnect.SHM_ON failure. What do you think @jparisu?

I could replicate the error in local, and it is due to time limit inside the test. Server B dies before Server A Data(Up) could arrive (this could happen because they are different processes and the time could not fit correctly).
Commit eProsima/Discovery-Server@d72162f in tests extends the time of the test. This should not fail again.

@EduPonz
Copy link

EduPonz commented Nov 16, 2021

Nice! One more CI run then with eProsima/Discovery-Server#45:

Build Status

@EduPonz
Copy link

EduPonz commented Nov 22, 2021

CI run with updated eProsima/Discovery-Server#45:

Build Status

@EduPonz EduPonz merged commit 4368260 into master Nov 22, 2021
@EduPonz EduPonz deleted the bugfix/ds-reconnection branch November 22, 2021 11:34
@EduPonz
Copy link

EduPonz commented Mar 22, 2022

@Mergifyio backport 2.1.x

mergify bot pushed a commit that referenced this pull request Mar 22, 2022
* Refs #12522: Fix minor ds errors

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: fix typo

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Client reconnection fix

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions to fix comment

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: uncrustify

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Apply new fix

Signed-off-by: jparisu <javierparis@eprosima.com>
(cherry picked from commit 4368260)

# Conflicts:
#	src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp
#	src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

backport 2.1.x

✅ Backports have been created

@rsanchez15
Copy link
Contributor

@Mergifyio backport 2.3.x

mergify bot pushed a commit that referenced this pull request Mar 22, 2022
* Refs #12522: Fix minor ds errors

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: fix typo

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Client reconnection fix

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions to fix comment

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: uncrustify

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Apply new fix

Signed-off-by: jparisu <javierparis@eprosima.com>
(cherry picked from commit 4368260)
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

backport 2.3.x

✅ Backports have been created

rsanchez15 added a commit that referenced this pull request Mar 23, 2022
Signed-off-by: RaulSanchez <raul@eprosima.com>
MiguelCompany pushed a commit that referenced this pull request Mar 25, 2022
* Discovery Server fix reconnection (#2246)

* Refs #12522: Fix minor ds errors

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: fix typo

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Client reconnection fix

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions to fix comment

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: uncrustify

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Apply new fix

Signed-off-by: jparisu <javierparis@eprosima.com>
(cherry picked from commit 4368260)

# Conflicts:
#	src/cpp/rtps/builtin/discovery/participant/PDPClient.cpp
#	src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp

* Refs #14221: Fix conficts backport #2246 to 2.1.x

Signed-off-by: RaulSanchez <raul@eprosima.com>

Co-authored-by: jparisu <69341543+jparisu@users.noreply.github.com>
Co-authored-by: RaulSanchez <raul@eprosima.com>
MiguelCompany added a commit that referenced this pull request Jul 21, 2022
* Discovery Server fix reconnection (#2246)

* Refs #12522: Fix minor ds errors

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: fix typo

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Client reconnection fix

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: apply suggestions to fix comment

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: uncrustify

Signed-off-by: jparisu <javierparis@eprosima.com>

* Refs #12522: Apply new fix

Signed-off-by: jparisu <javierparis@eprosima.com>
(cherry picked from commit 4368260)

* Fix build error

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Co-authored-by: jparisu <69341543+jparisu@users.noreply.github.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes don't reconnect to discovery server after restart [12742]
4 participants