Skip to content

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

Closed
HeartSaVioR wants to merge 1 commit intoapache:masterfrom
HeartSaVioR:MINOR-use-case-insensitive-map-for-kafka-continuous-source
Closed

[SPARK-28142][SS] Use CaseInsensitiveStringMap for KafkaContinuousStream#24942
HeartSaVioR wants to merge 1 commit intoapache:masterfrom
HeartSaVioR:MINOR-use-case-insensitive-map-for-kafka-continuous-source

Conversation

@HeartSaVioR
Copy link
Contributor

What changes were proposed in this pull request?

This patch addresses a missing spot which Map should be passed as CaseInsensitiveStringMap - KafkaContinuousStream seems to be only the missed one.

Before this fix, it has a relevant bug where pollTimeoutMs is always set to default value, as the value of KafkaSourceProvider.CONSUMER_POLL_TIMEOUT is kafkaConsumer.pollTimeoutMs which key-lowercased map has been provided as sourceOptions.

How was this patch tested?

N/A.

@HeartSaVioR
Copy link
Contributor Author

I skipped adding UT as similar changes for other data source are not covered by UT as well.

@HeartSaVioR
Copy link
Contributor Author

cc. @cloud-fan

@SparkQA
Copy link

SparkQA commented Jun 24, 2019

Test build #106811 has finished for PR 24942 at commit 8ca7d79.

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

@HyukjinKwon
Copy link
Member

Actually, @HeartSaVioR, can we file a JIRA since it fixes an actual bug?

@HeartSaVioR HeartSaVioR changed the title [MINOR][SS] Use CaseInsensitiveStringMap for KafkaContinuousStream [SPARK-28142][SS] Use CaseInsensitiveStringMap for KafkaContinuousStream Jun 24, 2019
@HeartSaVioR
Copy link
Contributor Author

Sure, just filed SPARK-28142 and changed the title. Thanks!

Copy link
Contributor

@gaborgsomogyi gaborgsomogyi left a comment

Choose a reason for hiding this comment

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

LGTM. This lower/natural case is a constant problem.

@gaborgsomogyi
Copy link
Contributor

As I'm implementing DSv2 batch source/sink I've pinpointed similar problems, so creating a PR...

@HyukjinKwon
Copy link
Member

Merged to master.

@dongjinleekr
Copy link
Contributor

@gaborgsomogyi @HyukjinKwon Could you have a look for #22282? It is related to this PR.

@HeartSaVioR
Copy link
Contributor Author

Thanks for reviewing and merging!

@HeartSaVioR HeartSaVioR deleted the MINOR-use-case-insensitive-map-for-kafka-continuous-source branch June 24, 2019 23:22
@gaborgsomogyi
Copy link
Contributor

@dongjinleekr I've some major items on my plate but coming back to the mentioned PR.

@gatorsmile
Copy link
Member

@HeartSaVioR @gaborfeher @HyukjinKwon We should not skip adding the unit test cases. Please add one. Thanks!

@HeartSaVioR
Copy link
Contributor Author

@gatorsmile Thanks for the suggestion. I'll see whether I can do it with small changes, and raise follow-up PR.

@HeartSaVioR
Copy link
Contributor Author

#24999 addressed UT. Please review the new patch. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments