-
Notifications
You must be signed in to change notification settings - Fork 573
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
Reduce duration of partition_movement_test from 25min to 8min #5238
base: dev
Are you sure you want to change the base?
Conversation
d019eec
to
90a704e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes focussed on the test itself are fine, but I worry that we are repeating the same mistake of adding new workloads rather than extending/adapting existing ones.
I'm totally on board with removing KafProducer, but when it comes to replacements, we already have FranzGoVerifiableProducer
which does pretty similar stuff to the new workloads added here (and I think the Java VerifiableProducer is not so different either?). If we were consolidating one one language I could understand, but the new workloads are in a mixture of languages too, so I'm left kind of uncertain about the motivation/direction.
@rystsov What's the status on this PR? Does it need review or are we waiting for some changes? |
d7084cb
to
d62a62e
Compare
@NyaliaLui I've addressed the comments and it waits for review |
@jcsp The new verifier follows a different approach to testing so extending the existing verifiers would require more work. Once this PR is merged I'll push for giving up on VerifiableProducer, VerifiableConsumer and KafProducer. I've described the key features of this verifier in the cover letter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like where this is going and the fact that the tool has helped detect a problem adds to the rw_worloads
value.
97d6305
to
43f2b83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Y'all the PR was ready to merge a while ago, I fixed the conflicts and it's ready to go. IMHO we may do an exception and let it go in despite the blockers. They aren't caused by the PR and the PR introduces a verifier to solve the "Consumer failed to consume up to offsets" ci-failure. What do you think?
LGTM
43f2b83
to
548608a
Compare
Moving test debs which change rarely up the docker file to prevent long rebuilds caused by frequently changed debs in the head of the dockerfile before them
we don't use kafka-verifier, removing the dead code
it's wasteful to download the same dependencies over and over introducing a verifiers project to unite the java projects to share debs
rw_verifier is a replacement for verified producer and consumer services. rw_verifier is a workload generator and an online linear and incremental consistency checker capable of running for hours without an overhead. The validation happens as the write / read go. No need to pass written/read data to the test over ssh for vali- dation. The app exposes control and metrics over http. So there is no need to eyeball the number of records: - start the workload - use the api to check that the app doing a progress at key points of the test - stop the app when the need without waiting for the remaining messages to pass
duration of partition_movement_test.py falls down from 14 minutes 15.077 seconds to 12 minutes 39.137 seconds
548608a
to
24f3e30
Compare
duration of partition_movement_test.py falls down from 12 minutes 39.137 seconds to 8 minutes 53.753 seconds
24f3e30
to
996620a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me.
- I think several files need a copyright header
- The set of features / benefits listed in the cover letter are impressive, but it's not clear why "Reduce duration of partition_movement_test from 25min to 8min" this is true. Is there some insight into the time savings?
it was true back in summer but now the benefits should be smaller, the major boost came from optimizing wait_until which was put in a separate PR since them the second best boost came from using logic like:
instead of:
I'll change the description to avoid the confusion |
|
||
static volatile long started_s = -1; | ||
static int workload_id_gen = 0; | ||
static synchronized int get_workload_id() { return workload_id_gen++; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we stick to the same naming convention for all functions/methods i.e. camelCase ?
|
||
HashMap<Integer, Queue<Long>> oids; | ||
HashMap<Long, OpInfo> ops; | ||
HashMap<Integer, Semaphore> rsems; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to make those variable names more descriptive ?
tests/java/verifiers/src/main/java/io/vectorized/reads_writes/Workload.java
Show resolved
Hide resolved
tests/java/verifiers/src/main/java/io/vectorized/reads_writes/Workload.java
Show resolved
Hide resolved
ConsumerRecords<String, String> records | ||
= consumer.poll(Duration.ofMillis(10000)); | ||
var it = records.iterator(); | ||
while (it.hasNext()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this to be equivalent with existing VerifiableConsumer it would have to commit offsets, tolerate errors (redeliveries) and use consumer groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that RWVerifier doesn't cover consumer groups. But I don't think full equivalence is the right goal, instead of having the verifier we should have a set of specialized verifiers aimed at a particular set of features. With a single verifier we tend not to question how good it is and what it actually covers resulting in a false sense of security.
For example the pair of VerifiableConsumer & VerifiableProducer:
- don't check if the consumed order matches the written order
- ignore time travel
- ignore when a consumer reads something what was never written
- ignore indecisive errors on produce
- has broken idempotency support (during rebalance a message is allowed to be consumed twice but the validation logic raise an error if there is any duplicate)
- doesn't validate if a partition is assigned to multiple clients at the same time
RWVerifier isn't affected by all but the last point. And it already found a problem which VerifiableConsumer & VerifiableProducer missed.
Instead of adding weak support of consumer groups to RWVerifier we should add another verifier exclusively targeting the consumer groups unaffected by the existing flaws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I want to have an example of how to write remove services without relying on piping the output via ssh so that when there is a new PR introducing an end_to_end based test I could point to a RWVerifier and ask the author to do something similar.
What do you think of copying partition_movement_test
into partition_movement_rw_test
and marking original deprecated until we introduce a verifier with the consumer group support? It sounds ugly but postponing this PR also has cost, we'll continue using the regular approach and we'll depend on it more and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thank you for explaining all the details i think there is no need to create duplicated tests the one we have now is enough, if the RWVerifier is going to be a goto tool to write EndToEnd tests what do you think about making it a little bit better documented in terms of usage/output and its code cleaned up so that the author of the new features will have an easier job.
ahh thanks. i remember that wait_until pr now. thanks! |
Cover letter
The new verifier is based on the verifiers we use in chaos tests. The key features of this verifier:
KafProducer
) which results in more organic usage pattern.app->driver
vsapp->kgo->driver
. The lower distance means it's easier to debug when something goes wrong.SIPartitionMovementTest.test_shadow_indexing
#4702)VerifiableProducer
) so no licensing questionsRelease notes