search

eclipse-uprotocol / up-transport-zenoh-rust

Rust client-side library to talk to the Zenoh implementation of uProtocol
Apache License 2.0
6 stars 5 forks source link

register_listener() with UUri::any() as origin filter #60

Closed AnotherDaniel closed 2 months ago

AnotherDaniel commented 3 months ago

I've run into an issue when using register_listener() in up-transport-zenoh. More specifically, the origin_filter parameter of that method: Seeing a parameter with that name in the context of a register_listener() method, I was happily passing in an UUri::any(), wanting to listen to any source - and get an error message saying "Wrong combination of source UUri and sink UUri".

After digging, I find that this messages is coming out of get_listener_message_type, which is complaining because the origin any-UUri I'm passing in does not contain a resource_id==0.

Now, there is several points I want to raise about this:

  1. User expectation When I'm passing an "any URI" into a register_listener function as an origin filter, I really expect this to work and the listener to listen to any URI. There's not much more to it really - this threw me for hours, I really didn't expect the error to be related to the origin filter, at all.

  2. Architectural layering As I see it, message types ("Request", "Notification", etc) - the failed identification of which are the root cause of the error - are a uprotocol level 2 thing, whereas UTransport::register_listener() is uprotocol level 1. So having level 2 concerns shine through into level 1 methods seems to be a design leak.

  3. User experience Whatever else we might agree to do here, I really would like to suggest a bit more specific error message texts for get_listener_message_type() ;-)

WDYT?

sophokles73 commented 3 months ago

I agree that the transport layer should not be concerned with the message exchange pattern being implemented on top of it.

evshary commented 3 months ago

@AnotherDaniel Thank you for the report. Your perspective is worthwhile.

  1. In fact, this is also the issue I face when I implement the transport. I filed the issue here, and finally created another function any_with_resource_id to solve it. Since there might be other users stepping into the trap, I think maybe I should update the up-spec and the code to accept different combinations of resource ID.
  2. Yes, now RPC is moved to L2, and in theory, L1 transportation should not care about this. However, if the transport layer also implemented the RPC mechanism, I still prefer to utilize the strength of the under-layer protocol. It should be more performant. WDYT?
  3. Nothing to say. I fully agree with you. Sorry to waste your time without providing the informative debug message. Will create a PR for this.
sophokles73 commented 3 months ago

However, if the transport layer also implemented the RPC mechanism, I still prefer to utilize the strength of the under-layer protocol. It should be more performant. WDYT?

But it doesn't, or does it? I cannot see where UPTransportZenoh implements RpcClient or RpcServer ...

evshary commented 3 months ago

Let me make it clearer. For the L1 API, there are several types of messages, Publish, Notification, Request and Response. For Publish and Notification, we use the pub and subscribe in Zenoh to implement. For Request and Response, we use the get and queryable in Zenoh to implement. Since they are not the same mechanisms, we need to figure out what kind of message type it is while invoking the L1 API. Then it comes out with the question Daniel asked.

Therefore, it's not related to RpcClient and RpcServer here.