zenoh
zenoh copied to clipboard
eclipse-zenoh
NBFTReliability
For now, my main blocker is not code (I haven't read much of it yet), but lack of documentation:
- What does NBFT mean?
- What are its contracts? (What problems are solved, under what conditions)
- How should it be used?
- How many caches?
- How should the caches be configured?
The filenames are also a bit awkward, why not group all NBFT things into a directory rather than prefixed files (at least for the ones in src)?
For the builder, I would ask why they don't just extend the standard builders, but I guess it's mostly because you might need access to data that the builders keep private. If possible, it would be nice to add a small trait to turn do sub_builder.reliable() and obtain the NFTReliableSubscriberBuilder. I only see this as possible if we can have the reliable builders have a SubscriberBuilder field, which might not be possible.
This last comment makes me think: maybe it would be useful to have a (forever unstable) way to break a builder into its component parts, so that zenoh-ext would less often need to re-implement the builders fully just to add one option.
For now, my main blocker is not code (I haven't read much of it yet), but lack of documentation.
Most of the answers (if they exist) are there: https://github.com/eclipse-zenoh/roadmap/blob/main/rfcs/ALL/Non%20Blocking%20Fault%20Tolerant%20Reliability.md Should we point there from the rustdoc ?
The filenames are also a bit awkward, why not group all NBFT things into a directory rather than prefixed files (at least for the ones in src)?
Sure
For the builder, I would ask why they don't just extend the standard builders, but I guess it's mostly because you might need access to data that the builders keep private.
That's indeed the main reason.
@OlivierHecart Since it's been a while ago, is the PR still alive? As a guard this week, just want to make sure no PR is forgotten. :smiley:
@OlivierHecart Please change your pull request's base branch to main (new default branch). And rebase your branch against main as it is missing a status check necessary to merge this pull request but which is only available on main.
@milyin I see this PR was mentioned in https://github.com/eclipse-zenoh/zenoh/issues/669 which is part of the next release. Would you mind linking this PR to the issue and adding it to the project roadmap? Bonus points if you use the release tag and set a timeline :D
@milyin I see this PR was mentioned in #669 which is part of the next release. Would you mind linking this PR to the issue and adding it to the project roadmap? Bonus points if you use the
releasetag and set a timeline :D
This implementation is going to be rewritten accordingly to requirements in https://github.com/eclipse-zenoh/zenoh/issues/669. This is definitely going to be started after the release, currently we don't have time to it. Converting the PR to draft
