-
Notifications
You must be signed in to change notification settings - Fork 89
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
Improve doc #690
Improve doc #690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Nice work! I learned a lot from reviewing it. The dfi documentation is especially valuable.
There are ambiguities which need clarification, though.
And by reading it, I found a serious design flaw of discovery_zeroconf
that needs to be addressed by another PR, which illustrates from another aspect the usefulness of this PR.
|
||
Because We will perform the mDNS query only using link-local multicast, so we set domain name default value "local". | ||
|
||
To reduce the operation of conversion between host name and address info. we set the address info to txt record, and set the host name and port to a dummy value("celix_rpc_dumb_host.local." and "50009"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content of TXT is set by the service provider. This design will force them to watch for address change, which violates the rationale of zeroconf and suggests a severe design flaw. I'll have a good read of RFC 6762 and 6763 this weekend before making further comments on this.
|
||
![remote_service_proxy_use_seq.png](diagrams/remote_service_proxy_use_seq.png) | ||
|
||
In the above process, each consumer of the remote service will have a different service proxy, because the service proxy needs to use the interface description file in the consumer (which may be a bundle) to serialize the service call information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not feel right. Currently, service descriptor is bundled by its various consumers.
THis is far less than ideal. We'd better let service discovery and RSA fetch service descriptors from service providers at runtime. Alternatively, we can generate the descriptor from the service header, i.e. solve #590.
In the same process, there should be one unique copy of service descriptor for each remote service instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same process, there should be one unique copy of service descriptor for each remote service instance.
I think this is the key issue. Only 1 unique descriptor for a service per process.
Ideally the consumer and provider are also aligned, but this is not always necessary. It is possible for the provider to provide newer version of the service, as long as it is backwards compatible.
For example a consumer using the following service:
#define FOO_SERVICE_NAME "foo"
#define FOO_SERVICE_VERSION "1.0.0"
struct {
void* handle;
int foo(const char* string);
} my_service
Can use a remote! provided service with the following service:
#define FOO_SERVICE_NAME "foo"
#define FOO_SERVICE_VERSION "1.1.0"
struct {
void* handle;
int bar(const char* string);
int foo(const char* string);
} my_service
Note that because bar
is added before foo
the updated service is not binary backwards compatible inside the same process. But for remote calls, as long as we can identity which method signature is called, different service versions is possible.
For the pubsub serializer handler different version is actual handled:
https://github.com/apache/celix/blob/rel/celix-2.4.0/bundles/pubsub/pubsub_utils/src/pubsub_serializer_handler.c#L237-L242
I also though there was a check on the literal descriptor content, but apparently not.
libs/dfi/README.md
Outdated
|
||
|**Identifier**|B |D |F |I |J |S |V |Z |b | i | j | s |P |t |N | | ||
|---------|---|------|-----|-------|-------|-------|----|--------------|-----|--------|--------|--------|------|------|---| | ||
|**Types**|char|double|float|int32_t|int64_t|int16_t|void|boolean(uint8)|uchar|uint32_t|uint64_t|uint16_t|void *|char *|int| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have *
, why we need t
for char *
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the t
is not really necessary anymore. IIRC I added the t
for 2 reasons:
- Although C uses a
char*
as string, in other languages a string type is often a special built-in type. - In the beginning of libdfi there was not support to specific if something was a pointer or not, so
t
was needed to specify achar*
, now*B
can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we still need char *
for plain char *
and t
for null-terminate c string.
|
||
*Type schema*: | ||
~~~ | ||
L(name);//shortcut for *l(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have *
, why we need *l
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the support for *
, I think we can drop L
and t
so that we do not have alternatives so "say" the same thing.
It's great to see some sorely missed documentation being added. I will try to review this PR this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work :). I do have some remarks, but overall this looks good.
|
||
To reduce the operation of conversion between host name and address info. we set the address info to txt record, and set the host name and port to a dummy value("celix_rpc_dumb_host.local." and "50009"). | ||
|
||
We set the instance name of the mDNS service as `service_name + hash(endpoint uuid)`. If there is a conflict in the instance name, mDNS_daemon will resolve it. Since the maximum size of the mDNS service instance name is 64 bytes, we take the hash of the endpoint uuid here, which also reduces the probability of instance name conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the service name still used, a UUID should be unique enough. Is this for readability/debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of UUID in instance name is explicitly discourage by RFC 6763:
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.
The rational is explained here.
Therefore, UUID should be removed from instance name altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see the usage of plant UML for some diagrams 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again nice to see some sequence diagrams. Especially for a complex sequence like a remote call over the remote service admin 👍
|
||
![remote_service_proxy_use_seq.png](diagrams/remote_service_proxy_use_seq.png) | ||
|
||
In the above process, each consumer of the remote service will have a different service proxy, because the service proxy needs to use the interface description file in the consumer (which may be a bundle) to serialize the service call information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same process, there should be one unique copy of service descriptor for each remote service instance.
I think this is the key issue. Only 1 unique descriptor for a service per process.
Ideally the consumer and provider are also aligned, but this is not always necessary. It is possible for the provider to provide newer version of the service, as long as it is backwards compatible.
For example a consumer using the following service:
#define FOO_SERVICE_NAME "foo"
#define FOO_SERVICE_VERSION "1.0.0"
struct {
void* handle;
int foo(const char* string);
} my_service
Can use a remote! provided service with the following service:
#define FOO_SERVICE_NAME "foo"
#define FOO_SERVICE_VERSION "1.1.0"
struct {
void* handle;
int bar(const char* string);
int foo(const char* string);
} my_service
Note that because bar
is added before foo
the updated service is not binary backwards compatible inside the same process. But for remote calls, as long as we can identity which method signature is called, different service versions is possible.
For the pubsub serializer handler different version is actual handled:
https://github.com/apache/celix/blob/rel/celix-2.4.0/bundles/pubsub/pubsub_utils/src/pubsub_serializer_handler.c#L237-L242
I also though there was a check on the literal descriptor content, but apparently not.
libs/dfi/README.md
Outdated
|
||
|**Identifier**|B |D |F |I |J |S |V |Z |b | i | j | s |P |t |N | | ||
|---------|---|------|-----|-------|-------|-------|----|--------------|-----|--------|--------|--------|------|------|---| | ||
|**Types**|char|double|float|int32_t|int64_t|int16_t|void|boolean(uint8)|uchar|uint32_t|uint64_t|uint16_t|void *|char *|int| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the t
is not really necessary anymore. IIRC I added the t
for 2 reasons:
- Although C uses a
char*
as string, in other languages a string type is often a special built-in type. - In the beginning of libdfi there was not support to specific if something was a pointer or not, so
t
was needed to specify achar*
, now*B
can be used.
|
||
*Type schema*: | ||
~~~ | ||
L(name);//shortcut for *l(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the support for *
, I think we can drop L
and t
so that we do not have alternatives so "say" the same thing.
I've finished reading RFC 6762 and 6763 (word by word), and have some further remarks on Large TXT RecordsAs previously pointed out by @pnoltes in an email:
The current design adopt the approach sketched by Service Instances with Multiple TXT Records
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. 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 SRVAs mentioned above, this basically defeats the purpose of zeroconf. Don't do this! It is OK to store URL in TXT records, but note that
URL/URI does not necessarily contain host and port. For more, check URI Syntax Components. Service Instance Name and Service SubtypesIf the application uses We only relies on the uniqueness of service instance name, otherwise It's mostly used for debugging.
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. |
Given that #710 has been merged, is this PR ready now? @xuzhenbao @pnoltes |
I will update this PR, please wait a few day. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
Hi @pnoltes
|
Thanks, LGTM. |
Improve document for
RSA
andDFI
It fixes #510.