Skip to content
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

[SPARK-32468][SS][TESTS][FOLLOWUP] Provide "default.api.timeout.ms" as well when specifying "request.timeout.ms" on replacing "default.api.timeout.ms" #29343

Closed
wants to merge 1 commit into from
Closed

[SPARK-32468][SS][TESTS][FOLLOWUP] Provide "default.api.timeout.ms" as well when specifying "request.timeout.ms" on replacing "default.api.timeout.ms" #29343

wants to merge 1 commit into from

Conversation

HeartSaVioR
Copy link
Contributor

What changes were proposed in this pull request?

This patch is a follow-up to fill the gap in #29272 which missed to also provide default.api.timeout.ms as well. #29272 unintentionally changed the behavior on Kafka side timeout which is incompatible with the test timeout. (default.api.timeout.ms gets default value which is 60 seconds, longer than test timeout.)

Why are the changes needed?

We realized the PR for SPARK-32468 (#29272) doesn't work as we expect. See #29272 (comment) for more details.

Does this PR introduce any user-facing change?

No, as it only touches the tests.

How was this patch tested?

Will trigger builds from Jenkins or Github Action multiple time and confirm.

@HeartSaVioR
[SPARK-32468][SS][TESTS][FOLLOWUP] Provide "default.api.timeout.ms" a…
4929439 
…s well when specifying "request.timeout.ms" to replace "default.api.timeout.ms"
@HeartSaVioR
Copy link
Contributor Author

Let's make sure Jenkins builds pass multiple times, and Github Action passes.

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@HeartSaVioR
Copy link
Contributor Author

cc. @zsxwing as he reviewed #29272
also cc. @HyukjinKwon as he indicated me the Kafka tests become flaky

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@SparkQA
Copy link

SparkQA commented Aug 4, 2020

Test build #127015 has finished for PR 29343 at commit 4929439.

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

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@SparkQA
Copy link

SparkQA commented Aug 4, 2020

Test build #127017 has finished for PR 29343 at commit 4929439.

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

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@HyukjinKwon HyukjinKwon mentioned this pull request Aug 4, 2020
Closed
@HyukjinKwon
Copy link
Member

Looks good given the description and explanation.

@HyukjinKwon
Copy link
Member

Thanks for working on this @HeartSaVioR.

@SparkQA
Copy link

SparkQA commented Aug 4, 2020

Test build #127018 has finished for PR 29343 at commit 4929439.

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

@SparkQA
Copy link

SparkQA commented Aug 4, 2020

Test build #127020 has finished for PR 29343 at commit 4929439.

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

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@gaborgsomogyi
Copy link
Contributor

Had a look, the explanation and solution makes sense. Thank you guys helping me out!

gaborgsomogyi
gaborgsomogyi approved these changes Aug 4, 2020
View reviewed changes
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.

@HyukjinKwon
Copy link
Member

I am going to merge this unblock other PRs.

@HyukjinKwon
Copy link
Member

Merged to master.

@HeartSaVioR
Copy link
Contributor Author

Thanks for the quick reviews & reaction!

@HeartSaVioR HeartSaVioR deleted the SPARK-32468-FOLLOWUP branch August 4, 2020 05:54
@SparkQA
Copy link

SparkQA commented Aug 4, 2020

Test build #127028 has finished for PR 29343 at commit 4929439.

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

jdcasale pushed a commit to palantir/spark that referenced this pull request Jun 22, 2021
@HeartSaVioR @rshkv
[SPARK-32468][SS][TESTS][FOLLOWUP] Provide "default.api.timeout.ms" a…
5329a42 
…s well when specifying "request.timeout.ms" on replacing "default.api.timeout.ms"

### What changes were proposed in this pull request?

This patch is a follow-up to fill the gap in apache#29272 which missed to also provide `default.api.timeout.ms` as well.  apache#29272 unintentionally changed the behavior on Kafka side timeout which is incompatible with the test timeout. (`default.api.timeout.ms` gets default value which is 60 seconds, longer than test timeout.)

### Why are the changes needed?

We realized the PR for SPARK-32468 (apache#29272) doesn't work as we expect. See apache#29272 (comment) for more details.

### Does this PR introduce _any_ user-facing change?

No, as it only touches the tests.

### How was this patch tested?

Will trigger builds from Jenkins or Github Action multiple time and confirm.

Closes apache#29343 from HeartSaVioR/SPARK-32468-FOLLOWUP.

Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaborgsomogyi gaborgsomogyi gaborgsomogyi approved these changes

Assignees
No one assigned
Labels
DSTREAM SQL STRUCTURED STREAMING
Projects
None yet
Milestone
No milestone
4 participants
@HeartSaVioR @SparkQA @HyukjinKwon @gaborgsomogyi