eclipse-zenoh
remove `new_timestamp` fn , `time` module, reworked plugin storage …
…manager to use session timestamp generator
These changes aim to remove the ability to create a timestamp outside of a Zenoh Session.
All timestamps and ID's associated must be connected to a Zenoh Session
LGTM. However before merging it would be good a list of expected sister PRs in backends/plugins/bindings in such a way we can merge them back-to-back across all the repos.
It looks like for the influx_DB plugin will need access to the session in the get_all_entries function in order to return a timestamp generated by the session,.
The other plugins hold Session in their state structs, but it makes sense to standardize using the use of the session passed in from the trait function.
I'm not sure about that... a plugin gets a Runtime from which a Session is created at startup and I believe it should keep it around for the whole lifetime. Without a Session the plugin can't really operate on Zenoh. Moreover, I believe get_all_entries should return the Timestamp from the stored one. Finally, I wonder why Option<OwnedKeyExpr> and not simply OwnedKeyExpr.
Let me summarize the problem with Influx_db:
The concept of "Timestamp" is that it's some unqiue and ordered identifier of sample. It's uniqueness is guaranteed by the fact, that at single moment single instance can emit only one sample. I.e. if identifier of sample consinsts of (time, zenoh_id), it's unique.
The storage receives samples from other sources and stores them with original timestamps. If timestamp doesn't exists, storage assigns it's own timestamp to it, with own session id.
get_all_entries returns latest timestamp for each stored key. If no key is stored, no timestamp exists. So the pair (None, Timestamp) have no sense, the return value from e.g. this stub function in Influx_DB should be just empty vector.
I.e. this
//putting a stub here to be implemented later
async fn get_all_entries(&self) -> ZResult<Vec<(Option<OwnedKeyExpr>, Timestamp)>> {
tracing::warn!("called get_all_entries in InfluxDBv2 storage");
let mut result: Vec<(Option<OwnedKeyExpr>, Timestamp)> = Vec::new();
let curr_time = zenoh::time::new_reception_timestamp();
result.push((None, curr_time));
return Ok(result);
}
}
should be just replaced by this:
//putting a stub here to be implemented later
async fn get_all_entries(&self) -> ZResult<Vec<(Option<OwnedKeyExpr>, Timestamp)>> {
return Ok(Vec::new());
}
}
In the case of InfluxDB, function get_all_entries is yet to be implemented.
https://github.com/ZettaScaleLabs/zenoh-backend-influxdb/pull/4
For now i will leave it as is until the dependent PR's are accepted.
And I will revert the changes to pass Session to get_all_entries
updating RocksDB backend now
Force push was to undo the changes adding session as an argument to the get_all_queries
PR to RocksDB changes:
https://github.com/eclipse-zenoh/zenoh-backend-rocksdb/pull/126
PR for InfluxDB: https://github.com/eclipse-zenoh/zenoh-backend-influxdb/pull/150 PR for RocksDB: https://github.com/eclipse-zenoh/zenoh-backend-rocksdb/pull/126
@Mallets The two repos that this effects are InfluxDB and RocksDB
In both cases after discussing with the Julien's, the code that used the generated timestamp would have broken replication semantics.
My understanding is that we want the hard semantic of a timestamp being stored in the Database as metadata, rather than generating one on call to get_all_entries.
and in the case of RocksDB after discussing with @JEnoch i have opened an issue suggesting a change to the startup of the plugin, in the case of data that does not have a timestamp, we can discuss further there. Let me know if any other changes need to happen to this PR.