[SPARK-28142][SS][TEST][FOLLOWUP] Add configuration check test on Kafka continuous stream by HeartSaVioR · Pull Request #24999 · apache/spark

HeartSaVioR · 2019-06-28T08:54:52Z

What changes were proposed in this pull request?

This patch adds missing UT which tests the changed behavior of original patch #24942.

How was this patch tested?

Newly added UT.

…tinuousStream

HeartSaVioR · 2019-06-28T08:57:49Z

cc. @gatorsmile @gaborgsomogyi @HyukjinKwon

@gaborgsomogyi Please feel free to leverage this and add a new test for micro-batch case in #24967.

gaborgsomogyi · 2019-06-28T09:06:42Z

@HeartSaVioR thanks for pulling me in, reviewing it through...

SparkQA · 2019-06-28T09:26:57Z

Test build #106994 has finished for PR 24999 at commit d7e4112.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class KafkaSourceProviderSuite extends SparkFunSuite with PrivateMethodTester

gaborgsomogyi

Basically look good and works properly.

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala

SparkQA · 2019-06-28T11:56:13Z

Test build #107000 has finished for PR 24999 at commit 8417fbe.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi

LGTM. Thanks for the change.

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala

HeartSaVioR · 2019-06-28T12:48:51Z

@gaborgsomogyi Ah, I think I just realized what you meant. The last commit reflects it. Sorry about confusion.

gaborgsomogyi · 2019-06-28T12:55:03Z

@HeartSaVioR that said I'm happy with 2 ways. Either generalize or split creation and value fetch.

SparkQA · 2019-06-28T13:06:50Z

Test build #107006 has finished for PR 24999 at commit 3d56605.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-28T13:15:13Z

Test build #107007 has finished for PR 24999 at commit 86c4002.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2019-07-02T05:34:08Z

Assuming @gaborgsomogyi thinks the recent change is also good as well...
@gatorsmile @HyukjinKwon Could you please review this and merge this in? Thanks in advance.

HyukjinKwon · 2019-07-02T06:07:19Z

Looks fine to me but I think we better have a confirmation from @gatorsmile since the comment is his.

HeartSaVioR · 2019-07-02T14:44:01Z

OK we can wait @gatorsmile . Thanks @HyukjinKwon for reviewing!

dongjoon-hyun · 2019-07-03T04:24:57Z

We had better use a meaningful title and description for every commits. Please don't use the same title in the follow-up PRs.

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala

HeartSaVioR · 2019-07-03T04:49:32Z

We had better use a meaningful title and description for every commits. Please don't use the same title in the follow-up PRs.

Thanks for the note! I just rewrote the title of this PR to be self-described.

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala

dongjoon-hyun

I'm not a big fan of the assumption for reusing of the upcoming PRs. But, I believe @HeartSaVioR 's suggestion. However, we should remove the repetition.

HeartSaVioR · 2019-07-03T05:47:09Z

Got it. I respect the direction on avoiding redundant code whenever possible, and try to inline if it's only used once. Here's an update.

I just added UT on what can be tested on micro-batch stream without #24967, and try to inline, deduplicate, etc. I guess @gaborgsomogyi wants to add new test which inspects other part of KafkaMicroBatchStream (which newly added UT doesn't cover), so once this gets merged, @gaborgsomogyi could extract some methods if he needs to reuse some parts of test.

HeartSaVioR · 2019-07-03T05:53:34Z

Sorry let me refine once more. I found missing piece on review comment.

SparkQA · 2019-07-03T06:13:27Z

Test build #107158 has finished for PR 24999 at commit 77da828.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-03T06:29:42Z

Test build #107160 has finished for PR 24999 at commit b1ccf87.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-03T06:36:09Z

Test build #107162 has finished for PR 24999 at commit 28a057d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-03T16:10:38Z

Thank you for updating, @HeartSaVioR . I'll review today again.

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala

SparkQA · 2019-07-04T03:22:27Z

Test build #107212 has finished for PR 24999 at commit 04b9374.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class KafkaSourceProviderSuite extends SparkFunSuite with PrivateMethodTester

dongjoon-hyun

+1, LGTM. Merged to master.

Thank you, @HeartSaVioR and @gaborgsomogyi .

HeartSaVioR · 2019-07-04T05:27:49Z

Thanks all for reviewing and merging!

[SPARK-28142][SS][FOLLOWUP] Use CaseInsensitiveStringMap for KafkaCon…

d7e4112

…tinuousStream

HeartSaVioR mentioned this pull request Jun 28, 2019

[SPARK-28142][SS] Use CaseInsensitiveStringMap for KafkaContinuousStream #24942

Closed

gaborgsomogyi reviewed Jun 28, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Outdated Show resolved Hide resolved

Generalize a bit

8417fbe

gaborgsomogyi approved these changes Jun 28, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Outdated Show resolved Hide resolved

HeartSaVioR added 2 commits June 28, 2019 21:40

Reduce over-engineering a bit

3d56605

Fixup - realize what review comment meant

86c4002

dongjoon-hyun added the STRUCTURED STREAMING label Jun 28, 2019

dongjoon-hyun changed the title ~~[SPARK-28142][SS][FOLLOWUP] Use CaseInsensitiveStringMap for KafkaContinuousStream~~ [SPARK-28142][SS][TEST][FOLLOWUP] Use CaseInsensitiveStringMap for KafkaContinuousStream Jul 3, 2019

dongjoon-hyun changed the title ~~[SPARK-28142][SS][TEST][FOLLOWUP] Use CaseInsensitiveStringMap for KafkaContinuousStream~~ [SPARK-28142][SS][TEST][FOLLOWUP] Add KafkaSourceProviderSuite Jul 3, 2019

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Show resolved Hide resolved

HeartSaVioR changed the title ~~[SPARK-28142][SS][TEST][FOLLOWUP] Add KafkaSourceProviderSuite~~ [SPARK-28142][SS][TEST][FOLLOWUP] Add configuration check test on Kafka continuous stream Jul 3, 2019

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Show resolved Hide resolved

dongjoon-hyun requested changes Jul 3, 2019

View reviewed changes

Add test for MicroBatchStream as well, inline single usage, etc.

77da828

HeartSaVioR added 2 commits July 3, 2019 14:58

More deduplication

b1ccf87

A bit more...

28a057d

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...l/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceProviderSuite.scala Show resolved Hide resolved

Reflect review comments

04b9374

dongjoon-hyun approved these changes Jul 4, 2019

View reviewed changes

dongjoon-hyun closed this in 4212a30 Jul 4, 2019

HeartSaVioR deleted the SPARK-28142-FOLLOWUP branch July 4, 2019 05:27

Conversation

HeartSaVioR commented Jun 28, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HeartSaVioR commented Jun 28, 2019

Uh oh!

gaborgsomogyi commented Jun 28, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HeartSaVioR commented Jun 28, 2019

Uh oh!

gaborgsomogyi commented Jun 28, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

SparkQA commented Jun 28, 2019

Uh oh!

HeartSaVioR commented Jul 2, 2019

Uh oh!

HyukjinKwon commented Jul 2, 2019

Uh oh!

HeartSaVioR commented Jul 2, 2019

Uh oh!

dongjoon-hyun commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

HeartSaVioR commented Jul 3, 2019

Uh oh!

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jul 3, 2019

Uh oh!

HeartSaVioR commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

dongjoon-hyun commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 4, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jul 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongjoon-hyun left a comment •

edited

Loading