search

apache / celix

Apache Celix is a framework for C and C++14 to develop dynamic modular software applications using component and in-process service-oriented programming.
https://celix.apache.org/
Apache License 2.0
165 stars 86 forks source link

Improve doc #690

Closed xuzhenbao closed 7 months ago

xuzhenbao commented 10 months ago

Improve document for RSA and DFI

It fixes #510.

pnoltes commented 10 months ago

It's great to see some sorely missed documentation being added. I will try to review this PR this week.

PengZheng commented 10 months ago

I've finished reading RFC 6762 and 6763 (word by word), and have some further remarks on discovery_zeroconf:

Large TXT Records

As previously pointed out by @pnoltes in an email:

One of the issue we had with mDNS, is that payload size is very limited and as result the remote service discovery was updated to a 2 step approach:

The current design adopt the approach sketched by Service Instances with Multiple TXT Records . For this to work reliably, care must be taken to make sure that IP fragmentation does not happen, because

Even on hosts that normally handle Ethernet "Jumbo" packets and IP fragment reassembly, it is becoming more common for these hosts to implement power-saving modes where the main CPU goes to sleep and hands off packet reception tasks to a more limited processor in the network interface hardware, which may not support Ethernet "Jumbo" packets or IP fragment reassembly.

A snapshot of mDNS wireshark capture will be very helpful to illustrate the current approach.

From the current documentation (I have not checked the implementation), it is not clear how we deal with packet loss. For service whose service advertisement fit in a single message, it is never a problem. But for services generating large TXT records, how do we guarantee to collect all associated TXT records.

Do we have a test case for packet loss? I fully understand it is challenging to test this. But if we do have one, it rocks.

Fixed Hostname and Port in SRV

As mentioned above, this basically defeats the purpose of zeroconf. Don't do this! Let just use the hostname of the OS. For RSA over HTTP, port should be the real port listened by the HTTP server. For RSA over shared memory, port could be 0.

It is OK to store URL in TXT records, but note that

The target host name and TCP (or UDP) port number of the service are given in the SRV record. This information -- target host name and port number -- MUST NOT be duplicated using key/value attributes in the TXT record.

URL/URI does not necessarily contain host and port. For more, check URI Syntax Components.

Service Instance Name and Service Subtypes

If the application uses rsa_shm alone, then only service instances exported by rsa_shm should be discovered. This is not the case (and less than ideal) for the current implementation: all service instances are enumerated by discovery_zeroconf. By using service subtypes, if the application only uses rsa_shm, discovery_zeroconf should search for something like _shm._sub._celix-rpc._udp rather than _celix-rpc._udp. For a user who want to list all exported services for debugging purpose, he/she can search for _celix-rpc._udp. Check Selective Instance Enumeration for more on service subtypes.

We only relies on the uniqueness of service instance name, otherwise It's mostly used for debugging. Fortunately, we only need to provide an informative name, mDNSResponder will help to guarantee its uniqueness:

The default name should be short and descriptive, and SHOULD NOT include the device's Media Access Control (MAC) address, serial number, or any similar incomprehensible hexadecimal string in an attempt to make the name globally unique. For discussion of why names don't need to be (and SHOULD NOT be) made unique at the factory, see Appendix D, "Choice of Factory-Default Names".

Service name alone is not informative enough. I suggest we include pid of the service provider, which is very important for debugging a misbehaved remote service.

A shell command or the existing mDNSResponder command tool should be a useful debugging tool.

PengZheng commented 7 months ago

Given that #710 has been merged, is this PR ready now? @xuzhenbao @pnoltes Note that parts of #699 may need to be updated into this.

xuzhenbao commented 7 months ago

Given that #710 has been merged, is this PR ready now? @xuzhenbao @pnoltes Note that parts of #699 may need to be updated into this.

I will update this PR, please wait a few day.

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.45%. Comparing base (423abb8) to head (7b3ea01). Report is 13 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #690 +/- ## ========================================== + Coverage 89.32% 89.45% +0.13% ========================================== Files 216 216 Lines 25153 25294 +141 ========================================== + Hits 22468 22627 +159 + Misses 2685 2667 -18 ```

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

xuzhenbao commented 7 months ago

Hi @pnoltes I have update this PR, it includes the following changes:

  1. Add document for dynamic IP mechanism.
  2. Add document for PR #699 in libdfi README.md
pnoltes commented 7 months ago

Hi @pnoltes I have update this PR, it includes the following changes:

1. Add document for dynamic IP mechanism.

2. Add document for PR [Feature/dfi cleanup #699](https://github.com/apache/celix/pull/699) in libdfi README.md

Thanks, LGTM.