Implement timestamp generators #1128

smoczy123 · 2024-11-20T20:59:43Z

To achieve parity with cpp-driver we need to implement client-side timestamp generators.
This pull request adds a TimestampGenerator trait and a MonotonicTimestampGenerator that implements it,
together with an extension to SessionBuilder that provides an ability to set a TimestampGenerator in Session
and use it to generate timestamps.

Fixes #1032

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

github-actions · 2024-11-20T21:06:41Z

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: a66210e

scylla/src/transport/timestamp_generator.rs

Lorak-mmk

Do other drivers (beside cpp-driver) have timestamp generators? If so, how do they implement them?
I'm asking because cpp-compatible implementation does not have to reside in Rust Driver: cpp-rust-driver can implement it in its codebase. We should think about what implementation(s) we want to provide to our users, not what cpp-rust-driver needs here.

Did you maybe test how long a call to MonotonicTimestampGenerator::next_timestamp() takes? The clock has a microsecond resolution and microsecond is a lot of time. It may be possible that if we call the generator often (like in your unit test), then it may always choose last + 1 branch, because we are still in the same microsecond as previous call. In that case after multiple calls the returned value may be far in the future compared to system clock. If there are multiple clients, it may cause issues.

smoczy123 · 2024-12-05T14:48:03Z

I've also checked Java and Python drivers, they implement it in the exact same way, taking the microsecond time since epoch. Unfortunately, next_timestamp() is quite fast, with last + 1 branch being used around 500 out of a 1000 iterations in the unit test. However, I think that such behavior is expected, that's why the default settings give the system clock 1 second to catch up. I think switching to a nanosecond timestamp would be manageable (i64 would support timestamps until around 2250), however I'm not sure that we would like to provide a behavior different to all other drivers implementing this generator.

Lorak-mmk

I don't think connection is the right layer to utilize the generator, imo it is more fitting for the session layer. Look into session.rs file, into functions like execute or run_query (cc: @wprzytula )
Nit: the commit messages have too long first lines, which makes github not render them correctly. Avoid having any lines longer than 70 characters, especially the first line (which should ideally have at most 50 characters).
The MonotonicTimestampGenerator struct could use more explanation in its doc comment. Please describe what it guarantees, how it behaves (errors, drifting etc).
On that front, documentation book should also be updated with info about timestamp generators. It should either be a new file in queries folder, or a new folder. @wprzytula @muzarski wdyt it the better place?

Lorak-mmk · 2024-12-10T13:01:03Z

scylla/src/transport/connection.rs

+        let mut timestamp = None;
+        if query.get_timestamp().is_none() {
+            if let Some(x) = self.config.timestamp_generator.clone() {
+                timestamp = Some(x.next_timestamp().await);
+            }
+        }


No need to clone here, a reference should be enough to generate a timestamp.

Lorak-mmk · 2024-12-10T13:04:01Z

scylla/src/transport/timestamp_generator.rs

+    warning_threshold_us: i64,
+    warning_interval_ms: i64,
+}


Currently you use raw integers for those durations, and you treat "0" as a special value.
The less error prone and more Rusty way is to use std::time::Duration to store thresholds / intervals, and use Option<std::time::Duration> instead of using a special value.

muzarski · 2024-12-10T16:25:01Z

* On that front, documentation book should also be updated with info about timestamp generators. It should either be a new file in `queries` folder, or a new folder. @wprzytula @muzarski wdyt it the better place?

Ideally, we could have a new directory with a file for each generator. However, if it turns out in multiple very short .md files, I don't see a problem with merging them into one file, and putting it under queries.

smoczy123 · 2024-12-10T16:42:23Z

My first thought was to put the timestamp generation in the session layer, however the functions I've changed in connection.rs are being called from multiple user-facing functions in the session layer directly and from even more indirectly. Generating the timestamps in the session layer would require us to generate a timestamp in every single one of those functions. Furthermore, if we decide to add more functions in the session layer, we would have to remember to add timestamp generation to them. I think this approach would be really bug-prone.

Lorak-mmk · 2024-12-11T13:26:02Z

My first thought was to put the timestamp generation in the session layer, however the functions I've changed in connection.rs are being called from multiple user-facing functions in the session layer directly and from even more indirectly. Generating the timestamps in the session layer would require us to generate a timestamp in every single one of those functions. Furthermore, if we decide to add more functions in the session layer, we would have to remember to add timestamp generation to them. I think this approach would be really bug-prone.

For normal queries modifying Sesion::run_query should be enough. The problem is with QueryPager, which has multiple constructors (QueryPager::new_for_prepared_statement, QueryPager::new_for_connection_execute_iter etc). I admit we lack a good place for this, so let's leave it in connection for now - improving this seems out of scope for this PR.

Added TimestampGenerator trait and MonotonicTimestampGenerator based on c++ driver's implementation

Also added an ability to set it through Session Builder

The timestamp generator in ConnectionConfig is set in Session::Connect()

Generated timestamp is only set if user did not provide one

Lorak-mmk · 2024-12-13T13:48:49Z

scylla/src/transport/timestamp_generator.rs

+/// Basic monotonic timestamp generator. Guarantees monotonicity of timestamps.
+/// If system clock will not provide an increased timestamp, then the timestamp will
+/// be artificially increased. If the warning_threshold (by default 1 second) is not None
+/// and the clock skew is bigger than its value, then the user will be warned about
+/// the skew repeatedly, with warning_interval provided in the settings (by default 1 second).
+/// warning_interval should not be set to None if warning_threshold is not None, as it is
+/// meaningless, if every generated timestamp over the skew threshold should warn, Duration::ZERO
+/// should be used instead.
+pub struct MonotonicTimestampGenerator {
+    last: AtomicI64,
+    last_warning: Mutex<Instant>,
+    warning_threshold: Option<Duration>,
+    warning_interval: Option<Duration>,
+}
+


🔧 Instead of using runtime checks and unwraps to verify that warning_interval is some if warning_threshold is some you can create a struct which will hold those 2 values (as Duration, not Option), and use Option. Then you'll have a compile-time guarantee that either both are present or both are not.

I'm not sure about the best name for this struct. MonotonicTimestampGeneratorWarnConf is a bit long.

Lorak-mmk · 2024-12-13T13:57:26Z

scylla/src/transport/timestamp_generator.rs

+/// Basic monotonic timestamp generator. Guarantees monotonicity of timestamps.
+/// If system clock will not provide an increased timestamp, then the timestamp will
+/// be artificially increased. If the warning_threshold (by default 1 second) is not None
+/// and the clock skew is bigger than its value, then the user will be warned about
+/// the skew repeatedly, with warning_interval provided in the settings (by default 1 second).
+/// warning_interval should not be set to None if warning_threshold is not None, as it is
+/// meaningless, if every generated timestamp over the skew threshold should warn, Duration::ZERO
+/// should be used instead.


🔧 It would be good to warn users that even though this generator prevents 2 queries from the same client from having same timestamps:

it does nothing to prevent 2 queries from different clients from having the same timestamp

it may be a potential point of contention: all queries go trough the atomic operations, potentially with a lot of retries (and the number of retries may increase the more queries there are).

🔧 We should actually benchmark the impact of this. @muzarski can we do that easily with cql-stress?

🤔 @smoczy123 An idea: could we provide another, simpler, impl of TimestampGenerator, that just uses SystemTime, without monotonicity guarantees. It would avoid contention and still allow users to use timestamps from the client machine.

Lorak-mmk · 2024-12-13T14:12:34Z

scylla/src/transport/timestamp_generator.rs

+                return u_cur;
+            } else if self.warning_threshold.is_some()
+                && last - u_cur > self.warning_threshold.unwrap().as_micros() as i64
+            {
+                let mut last_warn = self.last_warning.lock().await;


🔧 Instead of using unwrap you can use if let:

else if let Some(warn_threshold) = self.warning_threshold { // the rest of the logic }

Lorak-mmk · 2024-12-13T14:18:20Z

scylla/src/transport/timestamp_generator.rs

+    async fn next_timestamp(&self) -> i64 {
+        loop {
+            let last = self.last.load(Ordering::SeqCst);
+            let cur = self.compute_next(last).await;
+            if self
+                .last
+                .compare_exchange(last, cur, Ordering::SeqCst, Ordering::SeqCst)
+                .is_ok()
+            {
+                return cur;
+            }
+        }
+    }


🤔 When the client is under high load, won't this approach be a problem?
We should benchmark this, @muzarski could you help @smoczy123 with that?

The alternative approach would be to just put last under mutex, avoiding the retries. The added benefit of that is that you can store Instant under Mutex, which would simplify the code in compute_next.

I prepared a branch that uses rust-driver version from this PR: https://github.com/muzarski/cql-stress/tree/timestamp-gen.

@smoczy123, you can set the timestamp generator for c-s frontend in src/bin/cql-stress-cassandra-stress/main.rs in prepare_run function. The Session object is created in this function.

Commands for some simple workloads:

cql-stress-cassandra-stress write n=1000000 -pop seq=1..1000000 -rate threads=20 -node <ip addresses> cql-stress-cassandra-stress read n=1000000 -pop seq=1..1000000 -rate threads=20 -node <ip addresses>

First one will insert 1M rows to the databse, while the latter reads the rows and validates them. You can play around with the run parameters and options. You can also try running multiple loads simultaneously to simulate multi-client scenario.

To run scylla locally, you can see https://hub.docker.com/r/scylladb/scylla/. Then you can replace <ip addresses> in the commands above, with your scylla nodes' ips (comma-delimited list of ips).

If you stumble upon any problems, feel free to ping me.

Lorak-mmk · 2024-12-13T14:19:47Z

scylla/src/transport/timestamp_generator.rs

+pub struct MonotonicTimestampGenerator {
+    last: AtomicI64,
+    last_warning: Mutex<Instant>,
+    warning_threshold: Option<Duration>,
+    warning_interval: Option<Duration>,
+}


🤔
https://docs.rs/tokio/1.24.2/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use

To me this looks like a case where std mutex is totally appropriate. Actually, I'm not convinced that TimestampGenerator even has to be an async trait. @wprzytula @muzarski wdyt?

Agreed. I don't see any point in using asynchronous functions in the area of generating timestamps - these operations should be very quick and nonblocking.
The trait should not be async.

Lorak-mmk · 2024-12-13T14:47:19Z

scylla/src/transport/timestamp_generator.rs

+#[tokio::test]
+async fn monotonic_timestamp_generator_is_monotonic() {
+    const NUMBER_OF_ITERATIONS: u32 = 1000;
+
+    let mut prev = None;
+    let mut cur;
+    let generator = MonotonicTimestampGenerator::new();
+    for _ in 0..NUMBER_OF_ITERATIONS {
+        cur = generator.next_timestamp().await;
+        if let Some(prev_val) = prev {
+            assert!(cur > prev_val);
+        }
+        prev = Some(cur);
+    }
+}


🔧 Can you create another test to verify that it works when used from multiple threads concurrently?

Lorak-mmk · 2024-12-13T14:55:25Z

docs/source/queries/queries.md

 Driver supports all kinds of statements supported by ScyllaDB. The following tables aim to bridge between DB concepts and driver's API.
 They include recommendations on which API to use in what cases.

-## Kinds of CQL statements (from the CQL protocol point of view):
+## Kinds of CQL statements (from the CQL protocol point of view)

 | Kind of CQL statement | Single              | Batch                                    |
 |-----------------------|---------------------|------------------------------------------|


👍 Thanks for the changes in this file!

Lorak-mmk · 2024-12-13T14:55:55Z

docs/source/SUMMARY.md

Commit: "docs/source/SUMMARY.md"

❓ Why did you change indentation in docs/source/SUMMARY.md file?

Ah, my bad, automatic markdown linter changes must have gotten into the commit here and in the queries.md and I missed them, should the changes in both of these files be reverted (except the necessary ones of course)?

Changes in queries.md are fine. The formatting change in SUMMARY could be fine too, if there is some reason apart from personal preference to make it - that's why I asked the question. Afaik Markdown doesn't specify how many spaces should the indentation have, and for MdBook the example SUMMARY.md uses 4.

Lorak-mmk · 2024-12-13T14:58:13Z

docs/source/queries/timestamp-generators.md

+# Timestamp generators
+
+If you want to generate timestamps on the client side you can provide
+a TimestampGenerator to a SessionBuilder when creating a Session. Then
+every executed statement will have attached a new timestamp generated
+by the provided TimestampGenerator, as longas the statement did not
+already have a timestamp provided (e.g. by using the `TIMESTAMP` clause).
+


🔧 I don't think this is precise. The timestamp will be attached to the query, but the server will ignore it.
It would be good to describe the precedence here:

USING TIMESTAMP in the query itself

Timestamp manually set on the query

Timestamp from the generator

Lorak-mmk · 2024-12-13T14:59:04Z

docs/source/queries/timestamp-generators.md

+## Monotonic Timestamp Generator
+
+Most basic client-side timestamp generator. Guarantees monotonic timestamps
+based on the system clock, with automatic timestamp incrementation
+if the system clock timestamp would not be monotonic. If the clock skew
+exceeds warning_threshold of the generator (provided in the constructor, 1s by default)
+user will be warned with timestamp generation with warning_interval cooldown period
+(provided in the constructor, 1s by default) to not spam the user.


🔧 I wouldn't call it "most basic" - it is actually quite complicated. The most basic would be the one I proposed in another comment, which just naively uses SystemTime::now.

smoczy123 force-pushed the implement-timestamp-generator branch 2 times, most recently from 80fa6c4 to 3c0da90 Compare December 2, 2024 23:09

wprzytula reviewed Dec 3, 2024

View reviewed changes

scylla/src/transport/timestamp_generator.rs Outdated Show resolved Hide resolved

scylla/src/transport/timestamp_generator.rs Outdated Show resolved Hide resolved

smoczy123 force-pushed the implement-timestamp-generator branch 3 times, most recently from 811f0bf to 1d70873 Compare December 3, 2024 23:36

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 3, 2024

smoczy123 force-pushed the implement-timestamp-generator branch from 1d70873 to 0f151f1 Compare December 4, 2024 00:05

github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 4, 2024

smoczy123 force-pushed the implement-timestamp-generator branch 2 times, most recently from 6d1943a to 876d544 Compare December 5, 2024 00:14

smoczy123 marked this pull request as ready for review December 5, 2024 00:20

smoczy123 requested a review from wprzytula December 5, 2024 00:20

Lorak-mmk reviewed Dec 5, 2024

View reviewed changes

smoczy123 force-pushed the implement-timestamp-generator branch from 876d544 to feba1aa Compare December 5, 2024 14:50

wprzytula requested a review from Lorak-mmk December 8, 2024 08:22

Lorak-mmk requested changes Dec 10, 2024

View reviewed changes

smoczy123 added 4 commits December 11, 2024 22:55

transport/timestamp_generator: Added MonotonicTimestampGenerator

8496733

Added TimestampGenerator trait and MonotonicTimestampGenerator based on c++ driver's implementation

transport/session: Added timestamp generator to SessionConfig

a87a42e

Also added an ability to set it through Session Builder

transport/connection: Added timestamp generator to ConnectionConfig

533fc43

The timestamp generator in ConnectionConfig is set in Session::Connect()

transport/connection: Added logic for setting a timestamp

101d94c

Generated timestamp is only set if user did not provide one

smoczy123 force-pushed the implement-timestamp-generator branch from feba1aa to 4ad3cd7 Compare December 11, 2024 21:57

transport/session_test: Added tests for MonotonicTimestampGenerator

55b556d

smoczy123 requested a review from Lorak-mmk December 11, 2024 21:57

wprzytula requested a review from muzarski December 12, 2024 06:05

wprzytula added this to the cpp-rust-driver milestone Dec 12, 2024

smoczy123 force-pushed the implement-timestamp-generator branch 2 times, most recently from 69b7789 to a66210e Compare December 12, 2024 18:39

docs: Added timestamp generator docs

a66210e

Lorak-mmk requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement timestamp generators #1128

Implement timestamp generators #1128

smoczy123 commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading

Lorak-mmk left a comment

smoczy123 commented Dec 5, 2024

Lorak-mmk left a comment

Lorak-mmk Dec 10, 2024

Lorak-mmk Dec 10, 2024

muzarski commented Dec 10, 2024

smoczy123 commented Dec 10, 2024

Lorak-mmk commented Dec 11, 2024

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024 •

edited

Loading

muzarski Dec 16, 2024

Lorak-mmk Dec 13, 2024

wprzytula Dec 16, 2024

Lorak-mmk Dec 13, 2024 •

edited

Loading

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024 •

edited

Loading

smoczy123 Dec 14, 2024

Lorak-mmk Dec 14, 2024

Lorak-mmk Dec 13, 2024

Lorak-mmk Dec 13, 2024

Implement timestamp generators #1128

Are you sure you want to change the base?

Implement timestamp generators #1128

Conversation

smoczy123 commented Nov 20, 2024 • edited Loading

Pre-review checklist

github-actions bot commented Nov 20, 2024 • edited Loading

Lorak-mmk left a comment

Choose a reason for hiding this comment

smoczy123 commented Dec 5, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Dec 10, 2024

smoczy123 commented Dec 10, 2024

Lorak-mmk commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoczy123 commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading

Lorak-mmk Dec 13, 2024 •

edited

Loading

Lorak-mmk Dec 13, 2024 •

edited

Loading

Lorak-mmk Dec 13, 2024 •

edited

Loading