Skip to content

[SPARK-32482][SS][TESTS] Eliminate deprecated poll(long) API calls to avoid infinite wait in tests #29289

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.

Sign up for GitHub

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

Jump to bottom
Closed
gaborgsomogyi wants to merge 2 commits into from
Closed

[SPARK-32482][SS][TESTS] Eliminate deprecated poll(long) API calls to avoid infinite wait in tests #29289

gaborgsomogyi wants to merge 2 commits into from

Conversation

@gaborgsomogyi
Copy link
Contributor

@gaborgsomogyi gaborgsomogyi commented Jul 29, 2020

What changes were proposed in this pull request?

Structured Streaming Kafka connector tests are now using a deprecated poll(long) API which could cause infinite wait. In this PR I've eliminated these calls and replaced them with AdminClient.

Why are the changes needed?

Deprecated poll(long) API calls.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing unit tests.

@gaborgsomogyi
Copy link
Contributor Author

gaborgsomogyi commented Jul 29, 2020

Similar solution is planned on the driver side but that part is much more complicated and requires some time to answer all my questions and play around.

@SparkQA
Copy link

SparkQA commented Jul 29, 2020

Test build #126770 has finished for PR 29289 at commit 8748691.

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

@gaborgsomogyi
Copy link
Contributor Author

gaborgsomogyi commented Jul 29, 2020

cc @HeartSaVioR @zsxwing @xuanyuanking

@SparkQA
Copy link

SparkQA commented Jul 29, 2020

Test build #126773 has finished for PR 29289 at commit 910fa1b.

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

HeartSaVioR
HeartSaVioR approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

LGTM, as tests passed and the direction is exactly same as what we discussed on migrating poll(0).

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jul 29, 2020

I'll leave the PR a day to see any further input, and merge tomorrow. Please take a look, or leave comment if anyone needs some time to review this.

xuanyuanking
xuanyuanking approved these changes Jul 30, 2020
View reviewed changes
Copy link
Member

@xuanyuanking xuanyuanking left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
Add a reference for #28871 which also related to deprecated poll(long) API.

zsxwing
zsxwing approved these changes Jul 30, 2020
View reviewed changes
Copy link
Member

@zsxwing zsxwing left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jul 31, 2020

retest this, please

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jul 31, 2020

Looks Jenkins is out of the order. GitHub Actions build passed so should be good to merge and go ahead.

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jul 31, 2020

Thanks @HyukjinKwon for reminding! I'll go ahead merging this.

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jul 31, 2020

Thanks! Merged into master.

@AmplabJenkins
Copy link

AmplabJenkins commented Jul 31, 2020

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126850/
Test FAILed.

@gaborgsomogyi
Copy link
Contributor Author

gaborgsomogyi commented Jul 31, 2020

Thanks everybody to help me!

@gaborgsomogyi
Copy link
Contributor Author

gaborgsomogyi commented Jul 31, 2020

I'm proceeding further with SPARK-32032 but from tomorrow I'm going to have 3 weeks vacation so the PR will be delayed a bit...

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

Reviewers

@zsxwing zsxwing zsxwing approved these changes

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

@xuanyuanking xuanyuanking xuanyuanking approved these changes

Assignees

No one assigned

Labels

DSTREAM SQL STRUCTURED STREAMING

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gaborgsomogyi @SparkQA @HeartSaVioR @HyukjinKwon @AmplabJenkins @zsxwing @xuanyuanking