Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SPARK-25615][SQL][TEST] Improve the test runtime of KafkaSinkSuite: streaming write to non-existing topic #22671

Closed
wants to merge 2 commits into from
Closed

[SPARK-25615][SQL][TEST] Improve the test runtime of KafkaSinkSuite: streaming write to non-existing topic #22671

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a36250
[SPARK-25615] Improve the test runtime of KafkaSinkSuite: streaming -…
dilipbiswal Oct 8, 2018
16b23a8
Code review
dilipbiswal Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion external/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSinkSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,9 @@ class KafkaSinkSuite extends StreamTest with SharedSQLContext with KafkaTest {
var ex: Exception = null
try {
ex = intercept[StreamingQueryException] {
writer = createKafkaWriter(input.toDF(), withTopic = Some(topic))()
writer = createKafkaWriter(input.toDF(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set this inside createKafkaWriter? it seems like it's a generally good idea to not let anything block that long in tests. In fact KafkaContinuousSinkSuite sets it to 1000. We should standardize that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srowen Thank you. I thought about it initially, but went with a conservative fix.

it seems like it's a generally good idea to not let anything block that long in tests

I agree.

In fact KafkaContinuousSinkSuite sets it to 1000. We should standardize that too

Yeah.. wasn't sure on value to set. If we think 1 second is a good enough timeout setting, i can change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect 1000 may have been too low - that could cause flakiness if the test runner is slow or something and it legitimately takes a second or two for Kafka to respond. 10k definitely should work, and I'd be fine with 1k if we're really worried about test time here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think it's worth trying. Many Kafka tests are slow, and this could speed up many of them. What happens if it's 1 second everywhere? or even just 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srowen I have used 5 sec timeout for now and i have moved it to createKafkaWriter per ur suggestion. Pl.. let me know if you are okay. Currently there is only one test that make uses of this timeout value. So keep it conservatively at 5 secs ? FYI - i tried using a 1 sec timeout and this suite passes on my laptop.

withTopic = Some(topic),
withOptions = Map("kafka.max.block.ms" -> "10000"))()
input.addData("1", "2", "3", "4", "5")
writer.processAllAvailable()
}
Expand Down