[SPARK-4964] [Streaming] refactor createRDD to take leaders via map instead of array #4511
Conversation
|
Can one of the admins verify this patch? |
|
OK to test |
| override def toString(): String = { | ||
| s"Broker($host, $port)" | ||
| } | ||
|
|
|
I was surprised to see that the KafkaRDDSuite was not modified, then i realized that the KafkaRDDSuite doesnot use the public API to create RDDs. Can you please update KafkaRDDSuite to do that, so that the public APIs get tested? |
| @@ -40,43 +41,70 @@ class KafkaRDDSuite extends KafkaStreamSuiteBase with BeforeAndAfter { | |||
| tearDownKafka() | |||
| } | |||
There was a problem hiding this comment.
Can you make both tests reuse the kafka? All you need to do is setup and tear down Kafka in beforeAll and afterAll, respectively. This is for speed, setting up kafka takes painfully long time increasing test times.
There was a problem hiding this comment.
Sure, but I think there are two bigger things that could be done here
- Change the thread sleep in setupKafka to polling using eventually
- Change this group of tests to use nested suites, so kafka / zk setup is only done once
There was a problem hiding this comment.
True. But, ....
1 is a riskier change, that should be done later (not for this release), dont want to introduce flakiness
2. is a greater refactoring, that is definitely not worth doing now. I am not sure though nested testsuites are a good idea or not. Stickign all the Kafka tests in a single testsuite is something that we can consider, there are pros and cons.
For now at least we should reuse the kafka harness within the same testsuite. I did that for the other Kafka testsuites. The bigger question we can address in separate PRs meant for next release cycle.
|
Generally look pretty good. Just one major comment about reuseing the kafka harness across tests. Other than that, minor nits. Will merge once these are done and tests pass. |
|
ok to test. |
|
Test FAILed. |
|
test this. |
|
Test build #597 has started for PR 4511 at commit
|
|
Test build #27269 has started for PR 4511 at commit
|
|
Test build #597 has finished for PR 4511 at commit
|
|
Test build #27269 has finished for PR 4511 at commit
|
|
Test PASSed. |
…nstead of array Author: cody koeninger <[email protected]> Closes #4511 from koeninger/kafkaRdd-leader-to-broker and squashes the following commits: f7151d4 [cody koeninger] [SPARK-4964] test refactoring 6f8680b [cody koeninger] [SPARK-4964] add test of the scala api for KafkaUtils.createRDD f81e016 [cody koeninger] [SPARK-4964] leave KafkaStreamSuite host and port as private 5173f3f [cody koeninger] [SPARK-4964] test the Java variations of createRDD e9cece4 [cody koeninger] [SPARK-4964] pass leaders as a map to ensure 1 leader per TopicPartition (cherry picked from commit 658687b) Signed-off-by: Tathagata Das <[email protected]>
|
Thanks @koeninger I have merged this! |
No description provided.