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

Fix topic/service list inconsistency #415

Merged
merged 7 commits into from
Jul 13, 2023
Merged

Fix topic/service list inconsistency #415

merged 7 commits into from
Jul 13, 2023

Conversation

caguero
Copy link
Collaborator

@caguero caguero commented Jul 4, 2023

🦟 Bug fix

Fixes #400

Summary

Discovery periodically sends topic/service heartbeats to keep all the topics/services advertised by a given node alive in the network. These messages are received by other discovery processes that keep the topics/services up to date or remove them in the absence of heartbeat updates after some time.

In order to get the list of topics/services, gz needs to initialize the discovery. This means that discovery should wait some time to receive all the topics/services updates to create the topic/service list. The original intent was to wait two cycles (2 * heartbeatInterval) before we consider the discovery initialized. However the current code only waits one cycle, as we were using the amount of heartbeats sent as the condition. That's essentially one cycle, which wasn't the original intent. The simple fix wait two real heartbeat cycles now.

How to test it?

You can reproduce the issue without this patch. First, run the simulation:

gz sim shapes.sdf

Then, check that the gz service -l is consistent.

watch -g -n 3 'gz service -l | wc -l'

This command will exit when the output is different than the last command. It should fail after a few tries.
Then, repeat the testing procedure with this patch. The watch call should never finish, meaning the output of gz service -l is consistent.

Note: My plan is to merge this patch from Citadel, and then, forward port it.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero caguero added the 🏰 citadel Ignition Citadel label Jul 4, 2023
@caguero caguero changed the title Consider discovery initialized after two heartbeat cycles Fix topic/service list inconsistency Jul 4, 2023
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

changes look good. CI failures seem unrelated. Are the flakes or do they need to be addressed?

@caguero
Copy link
Collaborator Author

caguero commented Jul 12, 2023

changes look good. CI failures seem unrelated. Are the flakes or do they need to be addressed?

Probably unrelated with this PR but I'm compiling Citadel on Docker as I don't fully understand why that test is failing.

caguero added 6 commits July 12, 2023 18:49
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #415 (c277564) into ign-transport8 (cb1c95b) will decrease coverage by 0.05%.
The diff coverage is 88.10%.

❗ Current head c277564 differs from pull request most recent head 5b671de. Consider uploading reports for the commit 5b671de to get more accurate results

@@                Coverage Diff                 @@
##           ign-transport8     #415      +/-   ##
==================================================
- Coverage           83.61%   83.56%   -0.05%     
==================================================
  Files                  51       51              
  Lines                5035     5039       +4     
==================================================
+ Hits                 4210     4211       +1     
- Misses                825      828       +3     
Impacted Files Coverage Δ
include/gz/transport/Packet.hh 0.00% <0.00%> (ø)
...og/include/gz/transport/log/detail/QueryOptions.hh 100.00% <ø> (ø)
log/src/Batch.cc 89.28% <ø> (ø)
log/src/Descriptor.cc 89.65% <ø> (ø)
log/src/Descriptor.hh 100.00% <ø> (ø)
log/src/Log.cc 78.72% <ø> (ø)
log/src/Message.cc 100.00% <ø> (ø)
log/src/MsgIter.cc 80.43% <ø> (ø)
log/src/QualifiedTime.cc 97.81% <ø> (ø)
log/src/QueryOptions.cc 100.00% <ø> (ø)
... and 40 more

@caguero
Copy link
Collaborator Author

caguero commented Jul 13, 2023

I couldn't reproduce the issue with that test locally, so I've disabled it in Citadel.

@caguero caguero merged commit b3a8182 into ign-transport8 Jul 13, 2023
@caguero caguero deleted the issue_400 branch July 13, 2023 19:05
caguero added a commit that referenced this pull request Jul 13, 2023
* Consider discovery initialized after two heartbeat cycles

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>

Restore

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>

Disable

Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants