Conversation
github-actions
Bot
added
triage
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
mjsax
left a comment
mjsax
left a comment
There was a problem hiding this comment.
Couple of minor question. Overall, it's quite hard to read the diff. Would it be possible to split this into multiple PRs, to make individual rewrite easier to follow?
Eg, move a method from EOSTestDriver to SmokeTestDriver. Introduced helper class RecordProcessingState, introduce VerificationResult?
| final int maxRecordsPerKey, | ||
| final boolean eosEnabled) { | ||
| final Properties props = createConsumerProperties(kafka); | ||
| final KafkaConsumer<String, Number> consumer = new KafkaConsumer<>(props); |
| return partitions; | ||
| } | ||
|
|
||
| private static void verifyAllTransactionFinished(final KafkaConsumer<byte[], byte[]> consumer, |
There was a problem hiding this comment.
Seems we copied this from EosTestDriver? Why did we drop the repartition step?
If we use repartitioning in the EOS test, but not in the Smoke test, we should add a partition step to the smoke test, to not reduce test coverage.
|
|
||
| try (final KafkaConsumer<byte[], byte[]> eosConsumer = new KafkaConsumer<>(eosProps)) { | ||
| verifyAllTransactionFinished(eosConsumer, kafka); | ||
| return new VerificationResult(true, "EOS verification passed"); |
There was a problem hiding this comment.
If we are working with VerificationResult, would it be better to let verifyAllTransactionFinished return what we need? Or at least a boolean, to avoid unnecessary exception handling?
2 participants
After KAFKA-7944, the system test
streams_eos_test.pyis mostly redundant.