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

I've ran write and read workloads for a different numbers of threads in both single and multi client scenarios and I've seen no measurable difference in speed between using a monotonic timestamp generator and not using one. It seems like this does not cause issues with latency.

🌱 When we merge this PR, we need to open an issue to benchmark which implementation is faster under high load: current one, or simply using a mutex. No need to do this now because the implementation can be changed later.

🔧 In general I'd prefer time operations to use SystemTime / Duration / Instant when possible. This makes the code easier to follow (no need to remember which var is a duration and which is a timestamp) and less error prone. By refactoring this I noticed that there is actually no need to handle the case of clock being behind the unix epoch in most cases.

Here are my proposed changes (which assume that we use AtomicU64 instead of AtomicI64):

    // This is guaranteed to return a monotonic timestamp. If clock skew is detected
    // then this method will increment the last timestamp.
    fn compute_next(&self, last: SystemTime) -> SystemTime {
        let current = SystemTime::now();
        if current > last {
            return current;
        }
        if let Some(cfg) = self.config.as_ref() {
            // Panic safety: in the previous branch we return if "last" is earlier.
            let skew = last.duration_since(current).unwrap();
            if skew > cfg.warning_threshold {
                let mut last_warn = self.last_warning.lock().unwrap();
                let now = Instant::now();
                //
                if now >= last_warn.checked_add(cfg.warning_interval).unwrap() {
                    *last_warn = now;
                    drop(last_warn);
                    // Generated timestamps can only increase, so "last" can't be behind the epoch.
                    warn!(
                        "Clock skew detected. The current time ({:?}) was {:?} \
                    microseconds behind the last generated timestamp ({:?}). \
                    The next generated timestamp will be artificially incremented \
                    to guarantee monotonicity.",
                        current, skew, last
                    )
                }
            }
        }
        // Timestamps so far into the future that we can't represent them with
        // SystemTime is not the case we need to consider.
        last.checked_add(Duration::from_micros(1)).unwrap()
    }


    fn next_timestamp(&self) -> i64 {
        loop {
            let last_micros = self.last.load(Ordering::SeqCst);
            // Panic safety: the microseconds value is either 0 or was created from
            // a system time, so it is safe to turn it back into system time.
            let last_time = UNIX_EPOCH
                .checked_add(Duration::from_micros(last_micros))
                .unwrap();
            let cur = self.compute_next(last_time);
            // Initial value is 0, which will work here, and values from "compute_next"
            // can never decrease, so it is not possible for "cur" to be behind epoch.
            // Lossy conversion to u64 is fine - we don't need to care about timestamps
            // so far into the future.
            let cur_micros = cur.duration_since(UNIX_EPOCH).unwrap().as_micros() as u64;
            if self
                .last
                .compare_exchange(last_micros, cur_micros, Ordering::SeqCst, Ordering::SeqCst)
                .is_ok()
            {
                // Again, no need to consider so big timestamps.
                return cur_micros as i64;
            }
        }
    }

I change compute_next to operate on typed time-related types and moved integer conversions to next_timestamp. I change the warning to use debug impls for the types. Notice that we no longer need to separately handle to case of clock behind epoch.

Comparison on SystemTime operates at a nanosecond level, so this does not generate monotonic timestamps. We would have to get the actual timestamp to compare it properly.

Ah, but then we lose the panic safety, because current may be later that last, just not late enough to generate a new timestamp.

Comparison on SystemTime operates at a nanosecond level, so this does not generate monotonic timestamps. We would have to get the actual timestamp to compare it properly.

Ah, I did not think about that, sorry.

🔧 Why do you store i64 instead of u64? I don't think we intend to store times before epoch, right?

I thought it would be more elegant, as IIRC timestamps are i64

Yeah I forgot to delete this comment.
Neither of the types are perfect here. Protocol uses i64, but forbids negative values - another artifact of this protocol being designed with only Java in mind 🤦

Actually the AtomicU64 variant has the advantage that Duration::from_micros accepts u64, so I think it is the better choice in this case.
The trait should still use i64 - because that is what the protocol uses (even if it forbids negative values).