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.
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
[SPARK-32387][SS] Extract UninterruptibleThread runner logic from KafkaOffsetReader #29187
Conversation
Test build #126327 has finished for PR 29187 at commit
|
Test build #126328 has finished for PR 29187 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in overall. Left some comments majorly on leaving code comments.
external/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/KafkaOffsetReader.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/util/UninterruptibleThreadRunner.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/util/UninterruptibleThreadRunner.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/util/UninterruptibleThreadRunner.scala
Outdated
Show resolved
Hide resolved
external/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/KafkaOffsetReader.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except some nits.
core/src/test/scala/org/apache/spark/util/UninterruptibleThreadRunnerSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/util/UninterruptibleThreadRunnerSuite.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/apache/spark/util/UninterruptibleThreadRunnerSuite.scala
Outdated
Show resolved
Hide resolved
Thanks guys for the quick review, started to process the suggestions... |
Test build #126424 has finished for PR 29187 at commit
|
retest this please |
Filed SPARK-32416 and SPARK-32417. |
Test build #126432 has finished for PR 29187 at commit
|
Filed SPARK-32418 |
retest this, please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll wait for @zsxwing a bit to see whether he has more comments to add.
Test build #126441 has finished for PR 29187 at commit
|
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @gaborgsomogyi , @HeartSaVioR , @zsxwing , @xuanyuanking .
Merged to master for Apache Spark 3.1.0 on December 2020.
What changes were proposed in this pull request?
UninterruptibleThread
running functionality is baked intoKafkaOffsetReader
which can be extracted into a class. The main intention is to simplifyKafkaOffsetReader
in order to make easier to solve SPARK-32032. In this PR I've made this extraction without functionality change.Why are the changes needed?
UninterruptibleThread
running functionality is baked intoKafkaOffsetReader
.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing + additional unit tests.