Skip to content

[DO-NOT-MERGE][SPARK-33913][SPARK-33921][BUILD] Upgrade kafka to 2.7.0 and Upgrade Scala version to 2.12.12#30939

Closed
viirya wants to merge 3 commits intoapache:masterfrom
viirya:SPARK-33913
Closed

[DO-NOT-MERGE][SPARK-33913][SPARK-33921][BUILD] Upgrade kafka to 2.7.0 and Upgrade Scala version to 2.12.12#30939
viirya wants to merge 3 commits intoapache:masterfrom
viirya:SPARK-33913

Conversation

@viirya
Copy link
Copy Markdown
Member

@viirya viirya commented Dec 27, 2020 •
edited
Loading

What changes were proposed in this pull request?

This patch proposes to upgrade Kafka version to 2.7.0 and Scala 2.12 patch version to 2.12.12.

Why are the changes needed?

To catch up with latest Kafka bug fix and features: https://downloads.apache.org/kafka/2.7.0/RELEASE_NOTES.html

To catch up with bug fix and improvement of latest Scala 2.12 patch.

https://github.com/scala/scala/releases/tag/v2.12.11
https://github.com/scala/scala/releases/tag/v2.12.12

  • Performance improvements in the collections library, the compiler
  • Fix memory leak in runtime reflection's TypeTag caches
  • Fix some toX methods that could expose the underlying mutability of a ListBuffer-generated collection
  • ASM was upgraded to 7.3.1, allowing the optimizer to run on JDK 13+

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit test.

@github-actions github-actions Bot added the BUILD label Dec 27, 2020
@SparkQA

This comment has been minimized.

Sign in to view
viirya
viirya commented Dec 27, 2020
View reviewed changes
Comment thread external/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaTestUtils.scala
// ensure that logs from all replicas are deleted if delete topic is marked successful
assert(servers.forall(server => topicAndPartitions.forall(tp =>
server.getLogManager().getLog(tp).isEmpty)),
server.getLogManager.getLog(tp).isEmpty)),
Copy link
Copy Markdown
Member Author

@viirya viirya Dec 27, 2020

Choose a reason for hiding this comment

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

apache/kafka#8931

HyukjinKwon
HyukjinKwon previously approved these changes Dec 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 28, 2020

Test build #133413 has finished for PR 30939 at commit 2443950.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon HyukjinKwon dismissed their stale review December 28, 2020 01:10

Looks there are test failures.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

Yea, it's the same issue as embeddedkafka/embedded-kafka#202. We need to bump Scala patch version to match Kafka's.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

Created #30940 for upgrading Scala 2.12 patch version.

@github-actions github-actions Bot added the DOCS label Dec 28, 2020
@viirya viirya changed the title [SPARK-33913][BUILD] Upgrade kafka to 2.7.0 [SPARK-33913][SPARK-33921][BUILD] Upgrade kafka to 2.7.0 and Upgrade Scala version to 2.12.12 Dec 28, 2020
dongjoon-hyun
dongjoon-hyun requested changes Dec 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Hi, @viirya . Sorry, but, are you sure about Scala 2.12.12?

I commented on your original Scala 2.12.12 PR, too.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

Hi, @viirya . Sorry, but, are you sure about Scala 2.12.12?

I commented on your original Scala 2.12.12 PR, too.

* [#30940 (comment)](https://github.com/apache/spark/pull/30940#issuecomment-751581096)

Yea, I just saw it. If we don't want Scala 2.12.12, seems we cannot upgrade Kafka 2.7.0 too: #30940 (comment).

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

I'm going to see Jenkins test to verify it, although I already experimented locally.

@viirya viirya changed the title [SPARK-33913][SPARK-33921][BUILD] Upgrade kafka to 2.7.0 and Upgrade Scala version to 2.12.12 [DO-NOT-MERGE][SPARK-33913][SPARK-33921][BUILD] Upgrade kafka to 2.7.0 and Upgrade Scala version to 2.12.12 Dec 28, 2020
@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Dec 28, 2020 •
edited
Loading

No, @viirya . You may not notice the bug, because we detected the issues six month ago and made a Spark-side adaptation via SPARK-32436 on July 2020.

Please see the discussion here for the technical details and Scala side bug IDs. AFAIK, the bug is in Scala 2.12.12 and we are waiting for 2.12.13 .

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Dec 28, 2020

cc @srowen and @mridulm who were together on the original PR for Scala 2.12.12 bug fix.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

No, @viirya . You may not notice the bug, because we detected the issues six month ago and made a Spark-side adaptation via SPARK-32436 on July 2020.

Please see the discussion here for the technical details and Scala side bug IDs. AFAIK, the bug is in Scala 2.12.12 and we are waiting for 2.12.13 .

* #29231

I'm reading scala/bug#12096. I understood Scala 2.12.12 was skipped now. I just meant if we cannot upgrade to Scala 2.12.12, then upgrading Kafka 2.7.0 is blocked by it.

Then we may also wait for next Kafka version which uses Scala 2.12.13 (if there is).

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Dec 28, 2020

In short, the Scala compiler had a critical bug.

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Dec 28, 2020

Got it, @viirya . For testing purpose, there is no problem.

@srowen
Copy link
Copy Markdown
Member

srowen commented Dec 28, 2020

Right, I think we'd just have to skip this for now.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 28, 2020

Test build #133425 has finished for PR 30939 at commit 91a6ccd.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 28, 2020

Thanks for discussion.

@viirya viirya closed this Dec 28, 2020
@dongjoon-hyun dongjoon-hyun mentioned this pull request Jan 18, 2021
Closed
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 23, 2021

FYI, we adjusted the optimizer configs for Kafka to avoid the exact Scala patch version dependency issue: apache/kafka#10174

It will be part of Kafka 2.8.0.

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Feb 25, 2021

Thank you, @ijuma .

@viirya viirya deleted the SPARK-33913 branch December 27, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun requested changes

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

Assignees

No one assigned

Labels

BUILD DOCS SQL STRUCTURED STREAMING

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@viirya @SparkQA @dongjoon-hyun @srowen @ijuma @HyukjinKwon