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

Conform Zenoh spec to 1.5.8 #103

Merged

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Apr 3, 2024

The PR is to deal with

Still need to wait for #92 before merging (I need to add some changes)

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
@stevenhartley stevenhartley added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 4, 2024
@evshary
Copy link
Contributor Author

evshary commented Apr 8, 2024

I've also updated the workflow and added the requirement of arguments in constructors.
Let me implement it in up-client-zenoh-rust first before merging it.

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
@evshary
Copy link
Contributor Author

evshary commented Apr 11, 2024

@gregmedd Thanks for your review. Feel free to advise me on how to make the spec clearer. Sometimes I might ignore some premise while writing down the spec.

Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

see comments

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
@PLeVasseur
Copy link
Contributor

PLeVasseur commented Apr 24, 2024

Hey @evshary -- today in the MQTT meeting when discussing @sophokles73's proposal for MQTT, I thought this could also serve a nice purpose for Zenoh as well.

The idea is to include both the source and the destination UAuthorities s.t. we can easily determine when a message is a publish vs notification message and bring the message through.

I think we'd also modify s.t. when we say we want to "subscribe to all", what we really mean is we'd like to subscribe to all Request, Response, and Notification messages, and none of Publish messages.

Publish messages will be handled on a topic-by-topic basis based on what uSubscription tells us to forward, so we do not have a need to get "all" Publish messages.

Essentially, I think that if we modify the Zenoh KeyExpr from:
up://origin-authority/origin-entity/origin-version/origin-resource.instance
to include the destination authority as well:
up://origin-authority/dest-authority/origin-entity/origin-version/origin-resource.instance
then I think we can disambiguate.

So based on the issue I raised in this PR, I have a proposal:

Setup

A uEntity (origin-authority/origin-entity) creates a message with

  • source: up://origin-authority/origin-entity/origin-version/origin-resource.instance#message
  • sink: -

Subscribing uE

Registers a listener on origin UUri

  • up://origin-authority/origin-entity/origin-version/origin-resource.instance#message

which UTransport.registerListener maps to Zenoh key

  • up/ + origin-authority-id/ + empty/ + origin-entity-id/origin-version/origin-resource-id

The MQTT proposal would have the destination as /, but I understand that // is not valid in a Zenoh KeyExpr. We'd need empty to be something reasonable and consistent.

Subscribing uStreamer

Registers a listener on special origin-authority UUri to catch out-going messages coming from Zenoh, intended for SOME/IP

  • up://origin-authority/*/*/*

which UTransport.registerListener maps to Zenoh key

  • up/ + */ + origin-authority-id/ + */*/*

uStreamer receives messages and forwards

uStreamer catches the following message on SOME/IP UTransport:

  • up://origin-authority/origin-entity/origin-version/origin-resource.instance#message

uStreamer then uses UTransport.send which maps to Zenoh Key:

  • up/ + origin-authority-id/ + empty/ + origin-entity-id/origin-version/origin-resource-id

which would now not match for the uStreamer's registerListener() because we have discriminated between Publish vs Notification messages via the two slots:

  • first the source authority
  • second the sink authority

Overall WDYT?

@sophokles73
Copy link
Contributor

sophokles73 commented Apr 25, 2024

Subscribing uStreamer
Registers a listener on special origin-authority UUri to catch out-going messages coming from > Zenoh, intended for SOME/IP

up://origin-authority/*/*/*
which UTransport.registerListener maps to Zenoh key

up/ + */ + origin-authority-id/ + */*/*

@PLeVasseur
If the origin-authority-id represents the source of the messages, then I do not see how this Zenoh Key could possibly match any messages that are intended for SOME/IP, because the third segment in the key represents the destination (sink) of the message, not the source.

I guess it would help if, in your example, you would use terms other than origin-authority but instead clearly distinguish between the non-SOME/IP authority vs the SOME/IP authority that you want to route between ...

@evshary
Copy link
Contributor Author

evshary commented Apr 25, 2024

@PLeVasseur I guess I missed some context. I still can't understand why we need to have both the source and sink authority in the Zenoh key. If we don't use this way to discriminate between Publish and Notification, will we face any problems in uStreamer?

Another question is that if we use up://origin-authority/dest-authority/origin-entity/origin-version/origin-resource.instance in Notification, it means that the one receiving notifications can only receive data from origin-authority in this Zenoh key. If I want to receive notifications that possibly come from A, B, or C, I need to subscribe to three Zenoh keys separately.

  • up://A/dest-authority/origin-entity/origin-version/origin-resource.instance
  • up://B/dest-authority/origin-entity/origin-version/origin-resource.instance
  • up://C/dest-authority/origin-entity/origin-version/origin-resource.instance

Or maybe I mistake something?

---Edit---
Maybe I got your point. Is this your problem?
eclipse-uprotocol/up-transport-zenoh-rust#31 (comment)

I'm thinking of whether using allowed_origin in subscriber can solve the problem or not.
https://github.com/eclipse-zenoh/zenoh/blob/0.10.1-rc/zenoh/src/subscriber.rs#L474

@sophokles73
Copy link
Contributor

sophokles73 commented Apr 25, 2024

Another question is that if we use up://origin-authority/dest-authority/origin-entity/origin-version/origin-resource.instance in Notification,

Not sure if you are talking about a UUri here or the Zenoh key created from a UUri?

In any case, a Notification message would contain a source UUri like
up://origin-authority/origin-entity/origin-version/origin-resource.instance
and a sink UUri like
up://sink-authority/sink-entity/sink-version/sink-resource.instance

The Zenoh uTransport might turn that into Zenoh key
up/origin-authority/sink-authority/sink-entity/sink-version/sink-resource.instance

On the receiving side, a uEntity would subscribe to up://sink-authority/sink-entity/sink-version/sink-resource.instance which the Zenoh uTransport would turn into key
up/*/sink-authority/sink-entity/sink-version/sink-resource.instance so that it can receive Notifications from all origins.

@sophokles73
Copy link
Contributor

The problem with all of this is that UTransport.register_listener currently does not support indicating if I want to register a listener for Publish or other types of messages. This, however, would be necessary in order for the transport impl to be able to create the proper Zenoh key to match on, i.e. whether to fill the authority into the source-authority or the sink-authority segment of the key.

We had this discussion weeks ago already but never really came to a conclusion. Maybe it is time to create a corresponding PR in up-rust to add that flexibility to UTransport.

WDYT? @PLeVasseur @evshary

@PLeVasseur
Copy link
Contributor

We had this discussion weeks ago already but never really came to a conclusion. Maybe it is time to create a corresponding PR in up-rust to add that flexibility to UTransport.

WDYT? @PLeVasseur @evshary

Yeah I like the idea of being action-oriented here in some feature branches:

  1. defining an updated register_listener() proposal for discriminating between Publish and Notification in up-rust
  2. reflecting usage of new register_listener() API into up-client-zenoh-rust
  3. seeing how we all feel about it
  4. based on our thoughts and feelings, bringing a change to up-spec
  5. merge up-rust & up-client-zenoh-rust changes from steps 1. and 2. (with any feedbacks from 3. & 4.)

WDYT @sophokles73, @evshary?

If sounds good, who would like to take 1. and do some API design?

@PLeVasseur
Copy link
Contributor

@evshary

Maybe I got your point. Is this your problem?
eclipse-uprotocol/up-transport-zenoh-rust#31 (comment)

Yes, exactly.

I'm thinking of whether using allowed_origin in subscriber can solve the problem or not.
https://github.com/eclipse-zenoh/zenoh/blob/0.10.1-rc/zenoh/src/subscriber.rs#L474

There may be a Zenoh-specific way to solve this, but AFAIK this will also pose an issue to MQTT5, will it not @sophokles73?

And if so, it feels cleaner to be explicit in the implementation of register_listener() within up-rust (and more generally in the spec) on how to handle Notification vs Publish.

@evshary
Copy link
Contributor Author

evshary commented Apr 26, 2024

On the receiving side, a uEntity would subscribe to up://sink-authority/sink-entity/sink-version/sink-resource.instance which the Zenoh uTransport would turn into key
up/*/sink-authority/sink-entity/sink-version/sink-resource.instance so that it can receive Notifications from all origins.

OK, it makes sense to use * to listen to all notifications.

The problem with all of this is that UTransport.register_listener currently does not support indicating if I want to register a listener for Publish or other types of messages. This, however, would be necessary in order for the transport impl to be able to create the proper Zenoh key to match on, i.e. whether to fill the authority into the source-authority or the sink-authority segment of the key.

I always support the idea of introducing type while calling register_listener, just as we discussed it in the infamous #73
But, since we are also discussing the change of UUri now, how about introducing the type concept in UUri?
We can discriminate the requests and responses from others (publish & notification) based on the fact that they have special format in UResource.
Now we have:

  • request: name=rpc, instance=xxxx
  • response: name=rpc, instance=response

We can also have something like

  • publish: name=pub, instance=xxxx
  • notification: name=notify, instance=xxxx

WDYT, @sophokles73 @PLeVasseur

There may be a Zenoh-specific way to solve this, but AFAIK this will also pose an issue to MQTT5, will it not @sophokles73?

I know little about MQTT, but it seems possible.
https://stackoverflow.com/questions/66785306/paho-mqtt-client-how-to-ignore-messages-published-by-myself

@sophokles73
Copy link
Contributor

@evshary we are currently discussing to get rid of the long form completely and use numeric identifiers for the resource only. So, using such a naming convention would not really work. I also do not see, why we would need to introduce the message type into the UUri, given that a message already has the type in the UAttributes. I'd rather extend the UTransport interface to support specifying the message type. I'll give it a shot and create a corresponding PR in up-rust.

@sophokles73
Copy link
Contributor

I have created eclipse-uprotocol/up-rust#92 in the hope that this would allow the Zenoh client to use the source and sink in the Zenoh key ...

@evshary evshary marked this pull request as ready for review May 16, 2024 12:06
@evshary
Copy link
Contributor Author

evshary commented May 16, 2024

Hi @PLeVasseur @gregmedd
As a user and developer of up-client-zenoh, perhaps you can take a look at the changes and see whether it makes sense or not. I tried to align with the latest up-spec changes for 1.5.8. In the meantime, I'll implement it in up-client-zenoh-rust to see whether there is anything I missed.

Hi @sophokles73 To confirm some questions:

  1. Will we decide to use unserved characters for authority name? If not, I'll add how to escape special characters for Zenoh key in the spec.
  2. Is there any reserved word for authority name? I mean when we're using Publish, we need to keep sink authority empty and I'm not sure if this should be defined in uri spec or every protocol spec.

@sophokles73
Copy link
Contributor

Will we decide to use unserved characters for authority name? If not, I'll add how to escape special characters for Zenoh key in the spec.

Ah, I hadn't considered that yet. Let me check again ...

@sophokles73
Copy link
Contributor

Is there any reserved word for authority name? I mean when we're using Publish, we need to keep sink authority empty and I'm not sure if this should be defined in uri spec or every protocol spec.

Sorry, I do not understand your point. Can you elaborate?

@PLeVasseur
Copy link
Contributor

PLeVasseur commented May 16, 2024

Is there any reserved word for authority name? I mean when we're using Publish, we need to keep sink authority empty and I'm not sure if this should be defined in uri spec or every protocol spec.

Sorry, I do not understand your point. Can you elaborate?

@evshary is asking this because it is disallowed in a Zenoh key expression to have two slashes following one another, i.e. if it were a publish and in say, MQTT // could be used to represent that in the topic name, the same is not possible in Zenoh.

So I think the ask is -- is there some reserved or disallowed character or word in uProtocol that we could use instead? If say $ were disallowed in a uProtocol UAuthority, then a publish message when converted to a Zenoh key expression may instead contain /$/ to note that the sink UUri is empty.

@sophokles73
Copy link
Contributor

sophokles73 commented May 16, 2024

So I think the ask is -- is there some reserved or disallowed character or word in uProtocol that we could use instead? If say $ were disallowed in a uProtocol UAuthority, then a publish message when converted to a Zenoh key expression may instead contain /$/ to note that the sink UUri is empty.

Ok, the question is: how does the zenoh key look like for subscribing to Publish messages from source-authority, right?

Based on the proposal from a previous comment on this thread, it would be something like:
up/source-authority/{empty}/*/*/*

But {empty} cannot actually be empty, because Zenoh does not allow // in key expressions, right?

@PLeVasseur
Copy link
Contributor

Yes, exactly. Thanks for wording it much better 😅

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
@evshary
Copy link
Contributor Author

evshary commented May 17, 2024

@evshary is asking this because it is disallowed in a Zenoh key expression to have two slashes following one another, i.e. if it were a publish and in say, MQTT // could be used to represent that in the topic name, the same is not possible in Zenoh.

@PLeVasseur Thank you for the clarification. This is exactly what I'm asking 😆
I didn't know that MQTT doesn't have this limitation before. What I can think of is that I can choose the special character which is not included in sub-delims to achieve the same effect. Or maybe I need to use escape character to indicate empty. I guess it's fine for me to define it in Zenoh spec, but we need to confirm the scope of the legal character in authority name first.

sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="

@sophokles73
Copy link
Contributor

We only allow the authority to be a host

host          = IP-literal / IPv4address / reg-name
reg-name      = *( unreserved / pct-encoded / sub-delims )

which means that you should be able to use any character from gen-delims

gen-delims    = ":" / "/" / "?" / "#" / "[" / "]" / "@"

What about #?

@evshary
Copy link
Contributor Author

evshary commented May 17, 2024

These are unavailable characters in Zenoh key
image
I'm thinking of using [] to escape the characters.
For example,

  • [] means empty
  • [question] means ?

etc.

@sophokles73
Copy link
Contributor

sophokles73 commented May 17, 2024

My understanding is that [] will overlap with the

IP-literal    = "[" ( IPv6address / IPvFuture  ) "]"

production and might be mistaken for a malformed IPv6 address?

However, the : seems to be unambiguous. Maybe you could use ::?

@evshary
Copy link
Contributor Author

evshary commented May 17, 2024

Oops, you're right. But IPv6 also uses :, for example ::1 means localhost.
Maybe using {} or \ might be better.

@sophokles73
Copy link
Contributor

sophokles73 commented May 17, 2024

Oops, you're right. But IPv6 also uses :, for example ::1 means localhost. Maybe using {} or \ might be better.

A corresponding IPv6 address would still look like [::1] but I guess {} is the best candidate yet as these characters are not allowed anywhere in URIs 👍

@evshary
Copy link
Contributor Author

evshary commented May 20, 2024

At first sight, I don't see any reason why we shouldn't further restrict the reg-name production used to contain

I'm fine with that. It's also easier to me.

A corresponding IPv6 address would still look like [::1] but I guess {} is the best candidate yet as these characters are not allowed anywhere in URIs 👍

Great! I'll use {} to indicate empty.

Here is the initial proposal on how to escape special characters. I'll update it if we decide to restrict the reg-name.
https://github.com/eclipse-uprotocol/up-spec/pull/103/files#diff-540fb00cfbe38ece62de18b3a95419f71516df5fb08cc89402bf33235411ae7fR140

Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

In general, I wonder if you could simply use standard URL/Percent-Encoding for special characters?

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated
Comment on lines 101 to 102
The TTL only takes effect while sending RPC in the Zenoh implementation.
TTL **MUST** be mapped to the timeout configuration in Zenoh query.
Copy link
Contributor

@sophokles73 sophokles73 May 21, 2024

Choose a reason for hiding this comment

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

what about other messages that have timed out already on arrival? Would you still deliver those to registered listeners? Or send out messages in send() that have already timed out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I missed something, but is there any way for us to know when the message is sent? How does the receiver side know the message is timed out only with TTL?

Copy link
Contributor

Choose a reason for hiding this comment

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

The UUIDv8 defined in the specifications includes the timestamp from when the request message was generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I missed that.

what about other messages that have timed out already on arrival? Would you still deliver those to registered listeners? Or send out messages in send() that have already timed out?

Then we should check whether messages time out or not on arrival in the transport implementation. Let me update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... do you think we should drop the expired messages on the transport layer (Zenoh, MQTT..), or just passed to users and leave the decisions to them?

Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Thanks for continuing to pull this together @evshary!

Want to hear your feedback on a few points 🙂

up-l1/zenoh.adoc Outdated Show resolved Hide resolved
up-l1/zenoh.adoc Outdated
@@ -253,7 +240,8 @@ The way to transfer Response message is different from others, because Zenoh can
==== Message routing

1. A streamer that is interested in all incoming messages for dest-authority registers a listener for
* `up://dest-authority////`
* source: `-`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't leave the comment high up enough. I'm referring to this line.

With the separation coming between uP-L1 and uP-L2, I have a question: would the up-client-foo-rust libraries be responsible for / be expected to implement uP-L2? Or would they optionally implement uP-L2, but allow for the posibility of plugging in the UTransport implementation they made into e.g. the RpcClient "default" implementation we'd have in up-rust?

Tagging @sophokles73 in for this one as well to hear his thoughts.

up-l1/zenoh.adoc Outdated

4. which `UTransport.registerListener` maps to Zenoh key
* local (w/o dest-authority): `upl/` + `dest-resource-id + dest-entity-id + dest-version`
* remote (with dest-authority): `upr/` + `dest-authority-id/` + `dest-resource-id + dest-entity-id + dest-version`
* `up/` + `+++*+++/dest-authority/dest-entity/dest-version/dest-resource`
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this has been settled at this point, I just don't recall the conversation...

So for Notification messages, the latter parts of the Zenoh key (entity, version, resource) are pulled from the destination, not the source?

Would we want the ability to discriminate at the registerListener level as to which source the Notification is coming from? If that were the case, I suppose we'd have to put the latter parts (entity, version, resource) from the source and modify the registerListener example to show us using the source field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for Notification messages, the latter parts of the Zenoh key (entity, version, resource) are pulled from the destination, not the source?

Only Publish messages use source as the latter part of Zenoh key. Notification and Request only care about the destination.

Would we want the ability to discriminate at the registerListener level as to which source the Notification is coming from? If that were the case, I suppose we'd have to put the latter parts (entity, version, resource) from the source and modify the registerListener example to show us using the source field.

I don't quite get your point. Maybe retrieving the source information from uAttributes is enough?

@stevenhartley
Copy link
Contributor

@evshary today on the MQTT call we concluded that we need to construct the MQTT topic including the full source and sink URI information so that we can register for notifications that require sink and source. We register a listener for notifications passing the source UUri and the destination UUri (sink) so that the message is routed directly to the uE that consumes the notification message.

Here is more information:

Based on our live discussion just now, we have concluded that in order to support notifications correctly (that will have source and sink addresses) we need to expand and update the MQTT5 topic to be the following:

{client_identifier}/{source.authority_name}/{source.ue_id}/{source.ue_version_major}/{source.resource_id}/{sink.authority_name}/{sink.ue_id}/{sink.ue_version_major}/{sink.resource_id}

Below are some examples using this updated notation:

Specific Publish Topic: d/{source.authority_name}/{source.ue_id}/{source.ue_version_major}/{source.resource_id}/////
All (incoming) requests to a specific Method (what a service would want to do): d/+/+/+/+/{sink.authority_name}/{sink.ue_id}/{sink.ue_version_major}/{sink.resource_id}
Streamer registering to receive any sent notifications, requests, or responses destine to its device:
d/+/+/+/+/{sink.authority_name}/+/+/0
Specific Notification topic: d/{source.authority_name}/{source.ue_id}/{source.ue_version_major}/{source.resource_id}/{sink.authority_name}/{sink.ue_id}/{sink.ue_version_major}/0

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary evshary changed the title Update how UUri maps to Zenoh key. Conform Zenoh spec to 1.5.8 May 23, 2024
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Contributor Author

evshary commented May 23, 2024

@stevenhartley Updated. TBH, I'm not in favor of the complexity of Zenoh key or MQTT topic, but the majority rules.

I also tidied up the examples of Publish, Notification, and Request UUri mapping after referring to the MQTT spec.
@sophokles73 @PLeVasseur I would like to hear your comments. Is it clear enough to developers?

Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenhartley stevenhartley merged commit 05fdd5d into eclipse-uprotocol:main May 23, 2024
1 check passed
@evshary evshary deleted the zenoh_uuri_mapping branch May 24, 2024 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants