MINOR: Migrate verificationAllTransactionComplete from EOS system test to smoke test#20718
MINOR: Migrate verificationAllTransactionComplete from EOS system test to smoke test#20718mjsax merged 7 commits intoapache:trunkfrom
verificationAllTransactionComplete from EOS system test to smoke test#20718Conversation
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.
Overall LGTM. A few minor comments to simplify further: think we can just use a single consumer instead of creating two.
| consumer.seekToBeginning(partitions); | ||
| // Verify all transactions are finished before proceeding with data verification | ||
| if (eosEnabled) { | ||
| final Properties txnProps = new Properties(); |
There was a problem hiding this comment.
Seems this are the same config we setup above for props. Seems we could just reuse the ones from above?
| txnProps.put(ConsumerConfig.ISOLATION_LEVEL_CONFIG, IsolationLevel.READ_COMMITTED.toString()); | ||
|
|
||
| final VerificationResult txnResult; | ||
| try (final KafkaConsumer<byte[], byte[]> consumer = new KafkaConsumer<>(txnProps)) { |
There was a problem hiding this comment.
Seem we are creating a consumer here, and also below. Could we re-use the same consumer? Ie use a top level try (final KafkaConsumer<byte[], byte[]> consumer = new KafkaConsumer<>(...)) and nest if (eosEnabled) block inside this try-catch, as well as the other verification we do later?
| consumer.seekToBeginning(partitions); | ||
| private static VerificationResult preVerifyTransactions(final String kafka, final boolean eosEnabled) { | ||
| if (!eosEnabled) { | ||
| return null; |
There was a problem hiding this comment.
Why do we return null? Might be easier to return a VerificationResult with passed == true?
| System.out.println("FAILED"); | ||
| return txnResult; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
As above -- why do we convert a "passed" into null ? Kinda confusing.
| final int maxRecordsPerKey, | ||
| final boolean eosEnabled) { | ||
| final VerificationResult txnResult = preVerifyTransactions(kafka, eosEnabled); | ||
| if (txnResult != null) { |
There was a problem hiding this comment.
This does not seem to be easy to reason about? If txnResult is not null and passed, we only know that all transactions completed, but we cannot stop the verification yet -- we could only exit early if we get a "false" VerifcationResult.
This code does only make sense if one knows how preVerifyTransactions works, and it would return null for "passed == true" case, but it make the code hard to ready.
There was a problem hiding this comment.
Make sense, updated
|
Thanks for the PR. Merged to |
…est to smoke test (apache#20718) Migrate `verificationAllTransactionComplete` and add `VerificationResult` to smoke test utils and make `verificationAllTransactionComplete` return `VerificationResult` to avoid extra exception handling. Reviewers: Matthias J. Sax <matthias@confluent.io>
…est to smoke test (apache#20718) Migrate `verificationAllTransactionComplete` and add `VerificationResult` to smoke test utils and make `verificationAllTransactionComplete` return `VerificationResult` to avoid extra exception handling. Reviewers: Matthias J. Sax <matthias@confluent.io>
After apache#6382, the system test streams_eos_test.py is redundant. As in apache#20718, the verification logic has already been migrated, so we only need to delete the related system tests Reviewers: Matthias J. Sax <matthias@confluent.io>
2 participants
Migrate
verificationAllTransactionCompleteand addVerificationResultto smoke test utils and makeverificationAllTransactionCompletereturnVerificationResulttoavoid extra exception handling
Refactor the verify() method since it has complexity problem:
preVerifyTransactions: Encapsulates the logic forverifying EOS transactions before processing records.
pollAndCollect: Isolates the consumer polling loop,partition assignment, and event collection logic.
reportAndFinalize: Handles the calculation of results,console reporting, and final verification assertions.
PollResult: A private static helper class used topass state (events, processed counts, results) between the polling and
reporting phases.
Reviewers: Matthias J. Sax matthias@confluent.io