tests: introduce new test for compression #1218
Conversation
|
|
fruch
force-pushed
the
hackathon-if-compresssion-tests
branch
from
e8cf9c5 to
b7db588
Compare
|
and now, with the file :) |
| let request_rule = |tx| { | ||
| RequestRule( | ||
| Condition::or(Condition::RequestOpcode(RequestOpcode::Execute), Condition::RequestOpcode(RequestOpcode::Query)), | ||
| RequestReaction::noop().with_feedback_when_performed(tx), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
❓ Isn't it erroneous that you don't filter out the control connection? Depending on when the driver issues metadata queries on it and what responses it gets, the sum of frames' lengths can vary.
There was a problem hiding this comment.
I think that no all of the control parts are getting compressed, so they are less relevant
I can include everything back again, but then we'll need to re-calibrate the possible ranges of data length
There was a problem hiding this comment.
BTW, where are these ranges taken from? Did you calculate them manually? Or these are the values from Java driver tests?
There was a problem hiding this comment.
I've removed the filters, and calibrated the expected results
|
|
||
| async fn test_compression(compression: Option<Compression>, text_size: usize, expected_frame_total_size_range: std::ops::Range<usize>) { |
There was a problem hiding this comment.
🔧 Could you please add a docstring / comment explaining the semantics of all parameters and the logic inside?
There was a problem hiding this comment.
added a docstring (I think it's in the right convention)
fruch
force-pushed
the
hackathon-if-compresssion-tests
branch
2 times, most recently
from
17d6fe9 to
98bcb75
Compare
| let request_rule = |tx| { | ||
| RequestRule( | ||
| Condition::True, | ||
| RequestReaction::noop().with_feedback_when_performed(tx), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
🤔 What has changed compared to the previous version (Or(Opcode::Query,Opcode::Execute)) is that now we also count other CQL messages (Options, Startup, etc.). What I suggest is to instead keep the previous condition but AND it with Condition::not(Condition::ConnectionRegisteredAnyEvent)) - then we filter out the control connection's activity.
There was a problem hiding this comment.
It's not gonna be accurate anyhow, regardless of what filter we'll be doing
and it's the only thing we need to assess here, if it's compress or not, and we are using the estimated size (with or without control connection activity)
There was a problem hiding this comment.
o.k. I've took now the logic you suggested
calibrated again the ranges, and let's see if CI is fine with that.
I think the outcome is the same either way (i.e. it's just a estimated values, and won't be completely accurate)
fruch
force-pushed
the
hackathon-if-compresssion-tests
branch
2 times, most recently
from
775d7e9 to
9563038
Compare
| #[tokio::test] | ||
| #[cfg(not(scylla_cloud_tests))] | ||
| async fn should_execute_queries_without_compression_10mb() { | ||
| test_compression(None, 1024 * 10, 10000..19000).await; | ||
| } |
There was a problem hiding this comment.
Just to be sure: length is given in bytes, so 1024 * 10 is 10 kiB, not 10 MiB, correct? Because the name of the test is ...mb, and this confuses me.
There was a problem hiding this comment.
yes I was confusing both thing
it's doesn't make a big difference in this tests if it's MB or MiB
I hope it's a bit clearer now
|
The test has failed for Cassandra: |
fruch
force-pushed
the
hackathon-if-compresssion-tests
branch
from
9563038 to
58fcb7b
Compare
test are trying both algorithms (snappy, lz4), while counting the size of the frames coming into scylla for reference we also have one test doing the same without compression test main structure was borrowed from java-driver, the usage of proxy to count the packets is unique to this change.
fruch
force-pushed
the
hackathon-if-compresssion-tests
branch
from
58fcb7b to
58a623c
Compare
test are trying both algorithms (snappy, lz4), while counting the size of the frames coming into scylla
for reference we also have one test doing the same without compression
test main structure was borrowed from java-driver, the usage of proxy to count the packets is unique to this change.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.