-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-25288][Tests]Fix flaky Kafka transaction tests #22293
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
Conversation
|
Test build #95497 has finished for PR 22293 at commit
|
|
Test build #4314 has finished for PR 22293 at commit
|
|
Test build #4318 has finished for PR 22293 at commit
|
|
Test build #4311 has finished for PR 22293 at commit
|
|
Test build #4316 has finished for PR 22293 at commit
|
|
Test build #4312 has finished for PR 22293 at commit
|
|
Test build #4317 has finished for PR 22293 at commit
|
|
Test build #4315 has finished for PR 22293 at commit
|
|
Test build #4319 has finished for PR 22293 at commit
|
|
Test build #4310 has finished for PR 22293 at commit
|
|
Test build #4313 has finished for PR 22293 at commit
|
|
now this passed 11 times on Jenkins |
| waitUntilBatchProcessed, | ||
| CheckAnswer(), | ||
| WithOffsetSync(topic) { () => | ||
| WithOffsetSync(topicPartition) { () => |
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.
Its weird to have a hanging "5" in the thunk. Rather take the expected offset as part of the with. That is, WithOffsetSync(topicPartition, expectedOffset = 5) {
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.
Good suggestion!
|
I was afraid that this would be flaky. Glad you found a solution quickly. just one comment to improve code readability. Otherwise LGTM. Feel free to merge when you have addressed that change. |
|
Test build #4326 has finished for PR 22293 at commit
|
|
Test build #4321 has finished for PR 22293 at commit
|
|
Test build #4323 has finished for PR 22293 at commit
|
|
Test build #4322 has finished for PR 22293 at commit
|
|
Test build #4328 has finished for PR 22293 at commit
|
|
Test build #4325 has finished for PR 22293 at commit
|
|
Test build #4327 has finished for PR 22293 at commit
|
|
Test build #4320 has finished for PR 22293 at commit
|
|
Test build #4324 has finished for PR 22293 at commit
|
|
Test build #95505 has finished for PR 22293 at commit
|
|
Thanks! Merging to master. |
## What changes were proposed in this pull request? Here are the failures: http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaRelationSuite&test_name=read+Kafka+transactional+messages%3A+read_committed http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV1SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV2SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed I found the Kafka consumer may not see the committed messages for a short time. This PR just adds a new method `waitUntilOffsetAppears` and uses it to make sure the consumer can see a specified offset before checking the result. ## How was this patch tested? Jenkins Closes apache#22293 from zsxwing/SPARK-25288. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]>
## What changes were proposed in this pull request? Here are the failures: http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaRelationSuite&test_name=read+Kafka+transactional+messages%3A+read_committed http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV1SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV2SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed I found the Kafka consumer may not see the committed messages for a short time. This PR just adds a new method `waitUntilOffsetAppears` and uses it to make sure the consumer can see a specified offset before checking the result. ## How was this patch tested? Jenkins Closes apache#22293 from zsxwing/SPARK-25288. Authored-by: Shixiong Zhu <[email protected]> Signed-off-by: Shixiong Zhu <[email protected]>
What changes were proposed in this pull request?
Here are the failures:
http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaRelationSuite&test_name=read+Kafka+transactional+messages%3A+read_committed
http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV1SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed
http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.sql.kafka010.KafkaMicroBatchV2SourceSuite&test_name=read+Kafka+transactional+messages%3A+read_committed
I found the Kafka consumer may not see the committed messages for a short time. This PR just adds a new method
waitUntilOffsetAppearsand uses it to make sure the consumer can see a specified offset before checking the result.How was this patch tested?
Jenkins