Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove new_timestamp fn , time module, reworked plugin storage … #1188

Merged

Conversation

Charles-Schleich
Copy link
Member

…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

Mallets and others added 3 commits June 25, 2024 17:45
* Fix the README format.

Signed-off-by: ChenYing Kuo <[email protected]>

* Fix some out-dated contents.

Signed-off-by: ChenYing Kuo <[email protected]>

---------

Signed-off-by: ChenYing Kuo <[email protected]>
@Mallets
Copy link
Member

Mallets commented Jun 26, 2024

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.

@Charles-Schleich
Copy link
Member Author

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.

@Mallets
Copy link
Member

Mallets commented Jun 27, 2024

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.

@milyin
Copy link
Contributor

milyin commented Jul 2, 2024

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());
    }
}

@Charles-Schleich
Copy link
Member Author

Charles-Schleich commented Jul 2, 2024

In the case of InfluxDB, function get_all_entries is yet to be implemented.
ZettaScaleLabs/zenoh-backend-influxdb#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

@Charles-Schleich Charles-Schleich force-pushed the dev/1.0.0_timestamp_rework branch from 9adb2d4 to 89cf6e5 Compare July 2, 2024 14:08
@Charles-Schleich
Copy link
Member Author

Force push was to undo the changes adding session as an argument to the get_all_queries
PR to RocksDB changes:
eclipse-zenoh/zenoh-backend-rocksdb#126

@Mallets
Copy link
Member

Mallets commented Jul 3, 2024

@Charles-Schleich could you please resolve the merge conflicts?

@Charles-Schleich
Copy link
Member Author

@Mallets Done

@Charles-Schleich
Copy link
Member Author

Charles-Schleich commented Jul 3, 2024

@Charles-Schleich
Copy link
Member Author

Charles-Schleich commented Jul 4, 2024

@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.

@Charles-Schleich Charles-Schleich requested a review from JEnoch July 5, 2024 10:49
@JEnoch JEnoch merged commit 55557f9 into eclipse-zenoh:dev/1.0.0 Jul 5, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants