Fix/batch length #824

samuelorji · 2023-10-06T22:16:16Z

Pre-review checklist

Modify the type of the number of queries in a batch statement from i16 to u16. As well as add guards to prevent making server calls when the number of queries is over u16::MAX

This is following this conversation in the issue

Fixes: #820

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.

… from an i16 to a u16 according to the CQL protocol spec

…e server when the number of batch queries is greater than u16::MAX, as well as adding some tests

cvybhu

Thank you for the contribution!
Left some comments, please take a look.

cvybhu · 2023-10-06T22:56:43Z

scylla/Cargo.toml

@@ -60,6 +60,7 @@ criterion = "0.4" # Note: v0.5 needs at least rust 1.70.0
 tracing-subscriber = { version = "0.3.14", features = ["env-filter"] }
 assert_matches = "1.5.0"
 rand_chacha = "0.3.1"
+bcs = "0.1.5"


Let's not add new dependencies when it isn't necessary.

let mut key = vec![0]; serialize_into(&mut key, &(i as usize)).unwrap();

Can be written as:

let mut key = vec![0]; key.extend(i.to_be_bytes().as_slice());

bcs looks like a big dependency, it'd be better not to pull in a whole serialization library just for this test.

Thought so too, was trying to fully replicate the issue. Fixed now

cvybhu · 2023-10-06T23:00:46Z

scylla-cql/src/frame/types.rs

+impl From<Infallible> for ParseError {
+    fn from(_: Infallible) -> Self {
+        ParseError::BadIncomingData("Unexpected Infallible Error".to_string())
+    }
+}


Could you explain what this trait implementation does?
AFAIU Infallible is for things that can never happen, so why do we want to convert it to a ParseError?

https://doc.rust-lang.org/std/convert/enum.Infallible.html

It's for the conversion of u16 to a usize. I wanted to do a simple as but didn't want to modify the existing code as much. I think converting from the previous i16 to a usize would have failed with the TryFromIntError error, but with u16 -> usize, it really is in fact infallible

Ah okay, I understand now.
So we have a few pieces of code like this one:

let a: i16 = 123; let b: usize = a.try_into()?;

And after changing the i16 to u16 they no longer compile because try_into() has an Infallible error type.
We can implement a conversion from Infallible to ParseError to make it compile, but it's a bit hacky.

I think it would be better to replace the try_into()?with into(), like this:

let a: u16 = 123; let b: usize = a.into();

There is a an implentation of From<u16> for usize, so we can just use into() here.

cvybhu · 2023-10-06T23:02:59Z

scylla-cql/src/frame/types.rs

 pub fn write_short(v: i16, buf: &mut impl BufMut) {
    buf.put_i16(v);
 }

+pub fn write_u16(v: u16, buf: &mut impl BufMut) {
+    buf.put_u16(v);
+}


Maybe we should change write_short to accept u16.
AFAIU short refers to the CQL short type, which is a u16.
But we can do that in a separate PR as it will be a large change. Let's keep this one focused on the problem with Batches.

I agree about changing write_short to accept u16. There is only a small number of uses of this function in the code (I counted only six) so it should be easy to fix it right now.

The switch wasn't so simple, i had to change the type in some other structs that were linked in a way I don't understand yet. example. But as long as the tests pass, it should be fine.

cvybhu · 2023-10-06T23:05:02Z

scylla-cql/src/errors.rs

+    #[error("Number of Queries in Batch Statement has exceeded the max value of 65,536")]
+    TooManyQueriesInBatchStatement,


The maximum value of u16 is 65,535 - one less than 65,536.
https://doc.rust-lang.org/std/primitive.u16.html#associatedconstant.MAX

It'd also be nice to display the number of queries that the user has passed. This can be done by adding (usize) to the enum variant and displaying it with {0}.

Nice Catch. Fixed

cvybhu · 2023-10-06T23:08:57Z

scylla/src/transport/large_batch_statements_test.rs

+async fn create_test_session(session: Session, ks: &String) -> Session {
+    session
+        .query(
+            format!("CREATE KEYSPACE IF NOT EXISTS {} WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 1 }}",ks),


Please change SimpleStrategy to NetworkTopologyStrategy.
We are currently phasing out SimpleStrategy, in the future only NetworkTopologyStrategy will be allowed.
The query syntax is the same, only the name has to replaced.

cvybhu · 2023-10-06T23:14:52Z

scylla/src/transport/large_batch_statements_test.rs

+
+    let err = write_batch(&session, too_many_queries).await;
+
+    assert!(err.is_err());


Please make sure that the error is actually TooManyQueriesInBatchStatement.
I think this can be done using assert_matches!.

Thanks, knew there was a way to do this without having to implement PartialEq on everything 😢

cvybhu · 2023-10-06T23:19:57Z

scylla/src/transport/large_batch_statements_test.rs

+    }
+    keys
+}
+


TBH I don't really follow the logic of key prefixes.
Would it be possible to just insert numbers 1..n and then read them all from the table?
I'm a fan of keeping things as simple as possible.

modified the tests to be simpler, but still show the intent of batch queries

cvybhu

It looks like the CI has failed.
Please make sure that cargo check --examples --tests and cargo clippy --examples --tests don't produce any errors or warnings.

samuelorji · 2023-10-09T11:12:38Z

@cvybhu. I've made the change

samuelorji · 2023-10-10T14:34:06Z

@cvybhu , any help trying to get the CI to work, it seems to be failing for a myriad of reasons when it works perfectly on my machine.

now, I've added an #![allow(unused_imports)] to prevent that failure amongst other things

cvybhu · 2023-10-10T14:46:28Z

@cvybhu , any help trying to get the CI to work, it seems to be failing for a myriad of reasons when it works perfectly on my machine.

I saw that the CI is failing on cargo clippy. It's possible that you have an older version of rust installed and because of that it doesn't produce some of the warnings that the latest version does. CI always uses the latest version of stable rust (1.73.0 at the moment)

Please make sure that cargo --version and cargo clippy --version print 1.73.0:

$ cargo --version
cargo 1.73.0 (9c4383fb5 2023-08-26)
$ cargo clippy --version
clippy 0.1.73 (cc66ad4 2023-10-03)

If it doesn't, you have to update rust to get the latest warnings. If you installed it using rustup you can just run rustup update.

Another thing is that you have to pass the flags --examples --tests to cargo clippy. Without them it ignores all the code in tests and examples.

Make sure that you are running the same commands as the workflow does: https://github.com/scylladb/scylla-rust-driver/blob/e6d6d3e5bd97ec746e12640a3b861e3cbdffd404/.github/workflows/rust.yml#L29C1-L29C1

Although you can probably skip the --verbose flag to reduce the output size.

cvybhu · 2023-10-10T14:48:25Z

now, I've added an #![allow(unused_imports)] to prevent that failure amongst other things

It'd be better to remove the unused imports that clippy is complaining about.

samuelorji · 2023-10-10T15:47:40Z

@cvybhu , Thanks for the feedback, I upgraded my cargo and cargo clippy versions, and I think I was able to fix the clippy errors. Now, it seems like the test I wrote is a little flakey, although I don't know why. In the previous run, it ran fine, but now, it's failing on this quite simple assertion that runs fine locally for me

cvybhu · 2023-10-10T16:03:18Z

Now, it seems like the test I wrote is a little flakey, although I don't know why. In the previous run, it ran fine, but now, it's failing on this quite simple assertion that runs fine locally for me

It looks like the test passes on Scylla, but fails on Cassandra. scylla-rust-driver supports both databases, so we have to make sure that it passes on both of them.

You can try running this test against Cassandra instead of Scylla.

You can start a single Cassandra node using docker:

docker run --rm -it -p 9042:9042 -p 19042:19042 cassandra

and then run your test on this node and see what goes wrong.

samuelorji · 2023-10-10T16:04:18Z

Now, it seems like the test I wrote is a little flakey, although I don't know why. In the previous run, it ran fine, but now, it's failing on this quite simple assertion that runs fine locally for me

It looks like the test passes on Scylla, but fails on Cassandra. scylla-rust-driver supports both databases, so we have to make sure that it passes on both of them.

You can try running this test against Cassandra instead of Scylla.

You can start a single Cassandra node using docker:
docker run --rm -it -p 9042:9042 -p 19042:19042 cassandra
and then run your test on this node and see what goes wrong.

Thanks, will do now

…mpler

samuelorji · 2023-10-10T17:47:15Z

@cvybhu , I think I found the issue, the batch write result sometimes result in a write timeout error:

DbError(WriteTimeout { consistency: LocalQuorum, received: 0, required: 1, write_type: Simple }, "Operation timed out - received only 0 responses.")

The only way I could replicate this was by running a cassandra cluster instead of a single instance, and even then, it happened infrequently, but the times when the batch write didn't succeed, it was due to the write timeout, which could be caused by the large size of the data or busy nodes.

WDYT

cvybhu · 2023-10-10T17:54:32Z

@cvybhu , I think I found the issue, the batch write result sometimes result in a write timeout error:

Ah that makes sense, Scylla is the faster (better) database after all ;) 60k requests can take a moment to complete.

In that case let's increase the timeout so that Cassandra can complete the work. There used to be set_request_timeout function, but AFAIR this was replaced by ExecutionProfiles. So you'll have to create an ExecutionProfile with larger timeout and make the Batch statement use this ExecutionProfile.

samuelorji · 2023-10-10T17:56:09Z

@cvybhu , I think I found the issue, the batch write result sometimes result in a write timeout error:

Ah that makes sense, Scylla is the faster (better) database after all ;) 60k requests can take a moment to complete.

In that case let's increase the timeout so that Cassandra can complete the work. There used to be set_request_timeout function, but AFAIR this was replaced by ExecutionProfiles. So you'll have to create an ExecutionProfile with larger timeout and make the Batch statement use this ExecutionProfile.

Okay, lemme give that a try

samuelorji · 2023-10-10T18:27:10Z

@cvybhu I've made the change. Added the change as a different commit that should be eventually squashed

samuelorji · 2023-10-10T18:53:39Z

so weird how my cargo and cargo clippy versions are both:

$ cargo --version
cargo 1.73.0 (9c4383fb5 2023-08-26)
$ cargo clippy --version
clippy 0.1.73 (cc66ad4 2023-10-03)

and I'm running Clippy check an Format Check. and I'm not getting any error, but it's weirdly failing on CI 🤔

cvybhu · 2023-10-10T19:10:32Z

and I'm running Clippy check an Format Check. and I'm not getting any error, but it's weirdly failing on CI 🤔

Looks like everything is OK in the last version. Clippy check passes in the CI now.

It failed on Cassandra tests again. Maybe you have to make the timeout even larger.

Btw you could replace assert(batch_result.is_ok()) with batch_result.unwrap(). unwrap() prints the error message on failure, so we would be able to see the timeout message in CI logs.

samuelorji · 2023-10-10T19:34:14Z

Still a write timeout error even with 240 seconds

thread 'transport::large_batch_statements_test::test_large_batch_statements' panicked at 'called `Result::unwrap()` on an `Err` value: DbError(WriteTimeout { consistency: LocalQuorum, received: 0, required: 1, write_type: Simple }, "Operation timed out - received only 0 responses.")', scylla/src/transport/large_batch_statements_test.rs:33:26

piodul · 2023-10-10T19:41:05Z

Still a write timeout error even with 240 seconds

thread 'transport::large_batch_statements_test::test_large_batch_statements' panicked at 'called `Result::unwrap()` on an `Err` value: DbError(WriteTimeout { consistency: LocalQuorum, received: 0, required: 1, write_type: Simple }, "Operation timed out - received only 0 responses.")', scylla/src/transport/large_batch_statements_test.rs:33:26

Let's not introduce unit tests that are very long to execute. I suppose the goal is to just verify that batch serialization succeeds, right? Can we just call the serialization logic directly and test that it returns the expected error or succeeds?

Alternatively, you can see if using prepared statements in the batch helps - but a batch with 60k statements is abnormally large nonetheless, so I doubt the test will end in a reasonable time.

piodul · 2023-10-10T19:48:14Z

One more thing - the timeout that you observed occurs on the DB side, i.e. Cassandra itself returns a timeout error. Increasing the timeout on the driver side will not help - it only affects how long the driver waits for response. You would have to modify the configuration used to run Cassandra in CI.

samuelorji · 2023-10-10T19:50:51Z

One more thing - the timeout that you observed occurs on the DB side, i.e. Cassandra itself returns a timeout error. Increasing the timeout on the driver side will not help - it only affects how long the driver waits for response. You would have to modify the configuration used to run Cassandra in CI.

Okay, makes sense, because the check for the size of the statements occurs at the session level, will a single test that shows the expected error suffice ?

piodul · 2023-10-10T19:57:51Z

Okay, makes sense, because the check for the size of the statements occurs at the session level, will a single test that shows the expected error suffice ?

I think yes. It would be better to also have a test which shows that serializing a large batch that is just under the limit works, but to me it's even better not to have long tests or flaky tests (due to timeouts). I don't suspect that regression here is likely, so if it's hard to write such a test then I won't insist.

…hat writes complete during cassandra tests

cvybhu · 2023-10-11T22:40:03Z

Let's not introduce unit tests that are very long to execute. I suppose the goal is to just verify that batch serialization succeeds, right? Can we just call the serialization logic directly and test that it returns the expected error or succeeds?

@piodul I think it would be good to have a test that actually runs a batch with ~40k queries. Otherwise how do we know that it actually works? It will take a moment, but IMO it's better to have a few heavy tests rather than no tests at all.
In Pytest there are markers like @heavy_test and it's possible to run all tests except for the heavy ones for quick feedback. Maybe Rust has something similar?

Plus inserting 40k rows shouldn't take THAT long. AFAIR Scylla was capable of 100k inserts/s on a single shard, so it shouldn't take more than a few seconds to run this test. Cassandra might be worse, but we could just disable this test for Cassandra.
One more idea would be to insert the same row 40k times - that would all be a single mutation, so AFAIU there wouldn't be any memtable flushes, should be really fast.

cvybhu · 2023-10-11T23:02:40Z

Rust docs mention that there are two main types of tests - unit tests, which are next to the source code and integration tests, which are in the tests folder[1]. It's possible to run only unit tests using cargo test --lib [2].

I think we could add the test that runs a huge batch as an integration test. It matches the definition of an integration test, because it uses the library in the same way that an outside user would. Then we'd have a split - unit tests are quick, can be ran alone, while integration tests can be heavier, but can be skipped when we need only the quick tests.

[1] https://doc.rust-lang.org/book/ch11-03-test-organization.html#the-tests-directory
[2] rust-lang/cargo#8396

piodul · 2023-10-12T08:23:04Z

Let's not introduce unit tests that are very long to execute. I suppose the goal is to just verify that batch serialization succeeds, right? Can we just call the serialization logic directly and test that it returns the expected error or succeeds?

@piodul I think it would be good to have a test that actually runs a batch with ~40k queries. Otherwise how do we know that it actually works? It will take a moment, but IMO it's better to have a few heavy tests rather than no tests at all.

I tried running the test locally and it actually doesn't take that long (<1s to insert a batch, both with Scylla and Cassandra). I was concerned about not making the tests too long for the developers to execute them locally, but I don't think this one sticks out too much (it spends most of the time on schema changes, actually - this seems to be a problem with most of our existing tests).

I'm actually more worried about introducing flakiness to the CI.

In Pytest there are markers like @heavy_test and it's possible to run all tests except for the heavy ones for quick feedback. Maybe Rust has something similar?

Rust's default testing framework, sadly, doesn't have too many features, so I doubt it.

Plus inserting 40k rows shouldn't take THAT long. AFAIR Scylla was capable of 100k inserts/s on a single shard, so it shouldn't take more than a few seconds to run this test. Cassandra might be worse, but we could just disable this test for Cassandra.

Disabling the test for Cassandra is also an option.

One more idea would be to insert the same row 40k times - that would all be a single mutation, so AFAIU there wouldn't be any memtable flushes, should be really fast.

The batch should already result in a single mutation, it only modifies a single partition. I don't know about Cassandra, but Scylla also seems to struggle with batches this large - I've seen some large stalls in the logs related to applying mutation to the memtable.

samuelorji · 2023-10-13T10:13:04Z

@cvybhu @piodul to move forward, should reinstate the previous tests that does an actual write but then ignore the tests for cassandra as done here

piodul · 2023-10-13T10:26:49Z

@cvybhu @piodul to move forward, should reinstate the previous tests that does an actual write but then ignore the tests for cassandra as done here

I'm fine with this approach. @cvybhu ?

cvybhu · 2023-10-13T12:40:48Z

@cvybhu @piodul to move forward, should reinstate the previous tests that does an actual write but then ignore the tests for cassandra as done here

Sounds good to me 👍

cvybhu

Thanks, it looks nice.

I left a few more comments, please take a look, and then we can merge it :)

cvybhu · 2023-10-13T15:43:46Z

scylla-cql/src/frame/types.rs

+pub fn read_short(buf: &mut &[u8]) -> Result<u16, ParseError> {
+    let v = buf.read_u16::<BigEndian>()?;
+    Ok(v)
+}
+
+pub fn read_u16(buf: &mut &[u8]) -> Result<u16, ParseError> {
+    let v = buf.read_u16::<BigEndian>()?;
    Ok(v)
 }


I think the read_u16 isn't needed anymore, we have read_short that does the same thing. Let's remove read_u16.

cvybhu · 2023-10-13T15:52:15Z

scylla-cql/src/frame/value.rs

+        if self.values_num == u16::MAX {
            return Err(SerializeValuesError::TooManyValues);
        }


The message for TooManyValues has to be adjusted as well, currently it mentions i16::MAX requests.

#[error("Too many values to add, max 32 767 values can be sent in a request")] TooManyValues,

cvybhu · 2023-10-13T15:57:49Z

scylla/src/transport/large_batch_statements_test.rs

+}
+
+async fn write_batch(session: &Session, n: usize, ks: &String) -> Result<QueryResult, QueryError> {
+    let mut batch_query = Batch::new(BatchType::Logged);


An Unlogged batch might be faster to execute, it could speed up the test a bit.

cvybhu · 2023-10-13T15:59:15Z

scylla/src/transport/large_batch_statements_test.rs

+    let max_queries = u16::MAX as usize;
+    let batch_insert_result = write_batch(&session, max_queries, &ks).await;
+
+    assert!(batch_insert_result.is_ok());


Could you change it to batch_insert_result.unwrap()? unwrap() will print the error message when the test fails. assert! will only give us assertion failed, which isn't very helpful.

cvybhu · 2023-10-13T16:11:38Z

scylla-cql/src/frame/types.rs

+impl From<Infallible> for ParseError {
+    fn from(_: Infallible) -> Self {
+        ParseError::BadIncomingData("Unexpected Infallible Error".to_string())
+    }
+}


Ah okay, I understand now.
So we have a few pieces of code like this one:

let a: i16 = 123; let b: usize = a.try_into()?;

And after changing the i16 to u16 they no longer compile because try_into() has an Infallible error type.
We can implement a conversion from Infallible to ParseError to make it compile, but it's a bit hacky.

I think it would be better to replace the try_into()?with into(), like this:

let a: u16 = 123; let b: usize = a.into();

There is a an implentation of From<u16> for usize, so we can just use into() here.

cvybhu

Looks good to me.

@piodul do you want to take a final look? I'm ready to merge.

piodul

Some last nitpicks, but after fixing them - LGTM

piodul · 2023-10-17T07:11:44Z

scylla/src/transport/large_batch_statements_test.rs

+async fn create_test_session(session: Session, ks: &String) -> Session {
+    session
+        .query(
+            format!("CREATE KEYSPACE IF NOT EXISTS {} WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 }}",ks),


You can use CREATE KEYSPACE without IF EXISTS, the ks name is guaranteed to be unique.

piodul · 2023-10-17T07:12:12Z

scylla/src/transport/large_batch_statements_test.rs

+    session
+        .query(format!("DROP TABLE IF EXISTS {}.pairs;", ks), &[])
+        .await
+        .unwrap();


This is not needed, the test will create a new keyspace because ks is guaranteed to be unique.

piodul · 2023-10-17T07:17:37Z

scylla/src/transport/large_batch_statements_test.rs

+        let query = format!("INSERT INTO {}.pairs (dummy, k, v) VALUES (0, ?, ?)", ks);
+        let values = vec![key, value];
+        batch_values.push(values);
+        let query = Query::new(query);


How about using prepared statements instead? Prepare the statement once before creating the batch and then use it in append_statement: batch_query.append_statement(prepared.clone()). This should reduce the work needed by DB to process the batch.

samuelorji · 2023-10-17T13:19:31Z

@piodul thanks for the review. I've made the change, can you please have another look?

cvybhu

LGTM

samuelorji added 2 commits October 6, 2023 22:11

changed the type of the maximum number of statements in a batch query…

4c013e8

… from an i16 to a u16 according to the CQL protocol spec

add a guard when doing batch statements to prevent making calls to th…

772c46f

…e server when the number of batch queries is greater than u16::MAX, as well as adding some tests

samuelorji mentioned this pull request Oct 6, 2023

Incorrect error for batching #820

Closed

cvybhu suggested changes Oct 6, 2023

View reviewed changes

samuelorji requested review from cvybhu and piodul October 7, 2023 12:15

samuelorji force-pushed the fix/batch-length branch from 8dfdcdb to 1d0a55f Compare October 9, 2023 10:44

cvybhu suggested changes Oct 9, 2023

View reviewed changes

samuelorji force-pushed the fix/batch-length branch from 1d0a55f to 6e9e37b Compare October 9, 2023 11:12

samuelorji force-pushed the fix/batch-length branch 3 times, most recently from 5bc57b5 to 64c719d Compare October 10, 2023 14:32

samuelorji force-pushed the fix/batch-length branch from 64c719d to 5e731af Compare October 10, 2023 15:05

review feedback: Change short to u16 as well as modify tests to be si…

efb96fc

…mpler

samuelorji force-pushed the fix/batch-length branch from 5e731af to efb96fc Compare October 10, 2023 17:43

samuelorji force-pushed the fix/batch-length branch from fe621f5 to 5ce661c Compare October 10, 2023 18:49

samuelorji force-pushed the fix/batch-length branch from 5ce661c to f1f8e61 Compare October 10, 2023 19:15

review feedback: add request timeout to execution profile to enable t…

ec5cf68

…hat writes complete during cassandra tests

samuelorji force-pushed the fix/batch-length branch from f1f8e61 to ec5cf68 Compare October 11, 2023 15:35

review feedback: disable expensive batch write test for cassandra

a23e6eb

samuelorji requested a review from cvybhu October 13, 2023 12:49

cvybhu suggested changes Oct 13, 2023

View reviewed changes

samuelorji requested a review from cvybhu October 13, 2023 17:17

review feedback

b35346b

samuelorji force-pushed the fix/batch-length branch from 4b0f475 to b35346b Compare October 13, 2023 17:37

cvybhu approved these changes Oct 13, 2023

View reviewed changes

cvybhu mentioned this pull request Oct 13, 2023

test_coalescing might be flaky #828

Closed

piodul approved these changes Oct 17, 2023

View reviewed changes

review feedback, use prepared statements

6ecaa03

samuelorji force-pushed the fix/batch-length branch from aa43b64 to 6ecaa03 Compare October 17, 2023 13:13

piodul approved these changes Oct 17, 2023

View reviewed changes

cvybhu approved these changes Oct 17, 2023

View reviewed changes

cvybhu merged commit 82c1c99 into scylladb:main Oct 17, 2023
8 checks passed

		#[error("Number of Queries in Batch Statement has exceeded the max value of 65,536")]
		TooManyQueriesInBatchStatement,


		let err = write_batch(&session, too_many_queries).await;

		assert!(err.is_err());

Fix/batch length #824

Fix/batch length #824

Conversation

samuelorji commented Oct 6, 2023

Pre-review checklist

cvybhu left a 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

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

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

Choose a reason for hiding this comment

cvybhu Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvybhu left a comment • edited Loading

Choose a reason for hiding this comment

samuelorji commented Oct 9, 2023

samuelorji commented Oct 10, 2023

cvybhu commented Oct 10, 2023 • edited Loading

cvybhu commented Oct 10, 2023

samuelorji commented Oct 10, 2023

cvybhu commented Oct 10, 2023

samuelorji commented Oct 10, 2023

samuelorji commented Oct 10, 2023

cvybhu commented Oct 10, 2023

samuelorji commented Oct 10, 2023

samuelorji commented Oct 10, 2023

samuelorji commented Oct 10, 2023

cvybhu commented Oct 10, 2023 • edited Loading

samuelorji commented Oct 10, 2023

piodul commented Oct 10, 2023

piodul commented Oct 10, 2023

samuelorji commented Oct 10, 2023

piodul commented Oct 10, 2023

cvybhu commented Oct 11, 2023

cvybhu commented Oct 11, 2023 • edited Loading

piodul commented Oct 12, 2023

samuelorji commented Oct 13, 2023

piodul commented Oct 13, 2023

cvybhu commented Oct 13, 2023

cvybhu left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvybhu left a comment

Choose a reason for hiding this comment

piodul left a 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

samuelorji commented Oct 17, 2023

cvybhu left a comment

Choose a reason for hiding this comment

cvybhu Oct 6, 2023 •

edited

Loading

cvybhu left a comment •

edited

Loading

cvybhu commented Oct 10, 2023 •

edited

Loading

cvybhu commented Oct 10, 2023 •

edited

Loading

cvybhu commented Oct 11, 2023 •

edited

Loading