zenoh-plugin-ros2dds - Code questions and improvement forum

[Charles-Schleich]

Hello, i wanted a place where we could discuss a couple potential ways to reason about and potentially improve the code in the
https://github.com/eclipse-zenoh/zenoh-plugin-ros2dds Repository.

Disclaimer: I need to run the project locally, and experiment a little more with my suggestions / potential improvements, such that i have good evidence that they are actual improvements.

Question 1 lazy_static: possible improvement

Last time I used them, a lazy_static value is only initialized when it is first accessed.
This means it is possible to only access a lazy static value on the hot path, in some branch of the program that might not be run immediately after start up for a some time,
and it will mean that it may incur a potentially high initialization / allocation runtime-cost.
Potential Alternatives could be Once Cell
Once Lock
Up for discussion and for me to learn, do we care about explicit initialization of variables upfront, or do we not need to worry so much about initializing and doing all the mem-allocs at program start ?

Question 2 Unwraps / Expects!: Possible improvement.

There are a couple unwraps,
i am a big believer in that Developers should handle their invariants to the best of their ability within the constraints of development time and delivery time.
So if its a case of us needing to get code to be released faster / get to market sooner, I can somewhat understand the unwraps.
However It would be preferable, if we could have code with as few unwraps as possible, and ideally only .unwrap_or() invocations.
Anything that could be considered potentially dangerous, even if its known / thought to be explicitly a safe operation,
code gets changed by many hands in the future, and unwraps can be a potential cause for problems later down the road.
So it would be great to improve this aspect.
I haven't spent enough time with the code to know what is on the hot path and what is not on the hot path, how ever i am happy to be involved with as much of the clean up needed regarding any potential unwraps, and Expects.

Question 3 Error enum: Possible Improvement

Creation of a proper Error enum ?
Possibly using the crates
thiserror, and anyhow
Is there a really good argument to use Strings as as the Error Type in many functions return signature, or could we make a case for using a Sum Type / Enum to model our potential Error space,
Even if we end up wrapping errors that we don't yet know how to represent in a Unknown(String) variant of an enum, we could potentially at least to begin to model the possible error space for this bridge and Plugin.
I more or less see this as a way to better offer guarantees regarding failure, and for users to be able to recover more easily.

I am very keen to hear your thoughts, i hope that i have made myself well understood, and if there is anything that isn't clear then please let me know.

[Mallets]

A quick reply to your questions.

Question 1
One of the reasons of using lazy_static! was the possibility to override the default value via an environment variable (e.g. set via command line). In the main Zenoh repository, that was achieved via the zconfigurable! macro. But in the eclipse-zenoh/zenoh-plugin-ros2dds repo seems that Once Cell/Once Lock could be used as replacement.

Question 2
I agree. unwrap() should be only used in case of unrecoverable error. Everything else should be bublled-up or logged.

Question 3
In Zenoh, we have defined a ZError which carries additional information for debugging while retaining some good level of efficiency performance-wise.