zenoh icon indicating copy to clipboard operation
zenoh copied to clipboard

remove `new_timestamp(id)` standalone function, keep it only as a method of the `Session`

Open milyin opened this issue 1 year ago • 5 comments

Describe the release item

This will guarantee that artifitial timestamp with fake ZenohId is never created. But this requires rework of zenoh/plugins/zenoh-plugin-storage-manager/src/replica/snapshotter.rs and several plugins. Make final decision after discussion with @J-Loudet and @Mallets

milyin avatar Jun 21 '24 14:06 milyin

Currenly we could safely make the standalone new_timestamp() function internal

milyin avatar Jun 21 '24 14:06 milyin

Actually, Session is not the only type to expose an id, there are also Config (which should be the same as the session) and Hello (and maybe others). And Session also exposed peers/routers ids.

The question is then, should we allow timestamp generation only for the local id (from session/config), or do we allow to generate timestamp also for "foreign" ids (peers/routers). I don't think enabling it for "foreign" ids makes sense, but I don't know anything about timestamp in zenoh. By the way, if the problem is "fake" ZenohId, I think the signature should be changed to accept ZenohId directly instead of impl Into<TimestampId>.

Also, maybe we should hide these uhlc implementation details, provide our own Timestamp type, only compatible with ZenohId, not arbitrary uhlc::ID.

wyfo avatar Jun 24 '24 09:06 wyfo

My understanding is that we wanted to have a Timestamp with an Idea of the Session, and the current implementation with a ZenohId being set to 1 for new timestamps was a bug.

I agree with the point of hiding uhlc implementation details, a user should not have to create a NonZeroX in order to make a timestamp.

Charles-Schleich avatar Jun 24 '24 13:06 Charles-Schleich

To guarantee the correct behaviour of Zenoh and avoid API misusage/abuse we should not expose the possibility of generate timestamp for foreign ids. The role of ZenohId in the timestamp is to guarantee unicity in the system, if unicity is not guaranteed then the correct behaviour can't be guaranteed either.

Therefore, I suggest to have something like:

impl Session {
    fn new_timestamp(&self) -> Timestamp { .. }
}

Mallets avatar Jun 25 '24 08:06 Mallets

Removed Timestamp entirely, PR:remove+rework_timestamp If we arent exposing a session to the Storage Trait, then each backend that needs to generate a timestamp will have to do so via a Clone of Runtime that can be kept in the Struct that implements Storage.

RocksDB PR: https://github.com/eclipse-zenoh/zenoh-backend-rocksdb/pull/126

Charles-Schleich avatar Jun 25 '24 14:06 Charles-Schleich

Completed

Mallets avatar Jul 22 '24 16:07 Mallets