search

gazebosim / gz-transport

Transport library for component communication based on publication/subscription and service calls.
https://gazebosim.org
Apache License 2.0
29 stars 36 forks source link

Allow programmatic configuration of unicast relays. #497

Closed mbeards closed 1 month ago

mbeards commented 2 months ago

This change allows users to configure relays from code without having to setenv(GZ_RELAY).

🎉 New feature

Summary

This change allows users to configure relays from code without having to setenv(GZ_RELAY).

Test it

This can be tested by starting a Node across a boundary where multicast isn't enabled, and setting the relay IP via NodeOptions.

Checklist

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.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 17.64706% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.64%. Comparing base (eac2e69) to head (9b823d8). Report is 16 commits behind head on gz-transport13.

:exclamation: Current head 9b823d8 differs from pull request most recent head 472dde4. Consider uploading reports for the commit 472dde4 to get more accurate results

Files Patch % Lines
include/gz/transport/Discovery.hh 0.00% 9 Missing :warning:
src/NodeOptions.cc 40.00% 3 Missing :warning:
src/Node.cc 33.33% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-transport13 #497 +/- ## ================================================== - Coverage 87.69% 87.64% -0.05% ================================================== Files 59 59 Lines 5704 5716 +12 ================================================== + Hits 5002 5010 +8 - Misses 702 706 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mbeards commented 2 months ago

My main problem with this patch is that so far NodeOptions are all options that affect a single node. However, adding relays will have a global impact in all the nodes.

Agreed that this is weird / gross. I guess ideally I'd want to have accessors for the global Discovery instances, but threading that through seems difficult.

I understand that we don't have a way to set global options besides using environment variables right now. Perhaps an alternative could be to not use NodeOptions and just create an AddGlobalRelay() / GlobalRelays() functions in Node to preserve the semantics of NodeOptions? What do you think?

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}
caguero commented 2 months ago

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}

Probably AddGlobalRelay() and GlobalRelays().

mbeards commented 2 months ago

You're thinking something like the following?

class Node {
  // ...
  public: static void AddGlobalRelay(const std::string&);
  public: static std::vector<std::string>& Relays();
  // ...
}

Probably AddGlobalRelay() and GlobalRelays().

Done in https://github.com/gazebosim/gz-transport/pull/498