Skip to content

[KYUUBI #4713][TEST] Fix false positive result in SchedulerPoolSuite#4714

Merged
pan3793 merged 2 commits intoapache:masterfrom
huangzhir:fixtest-schedulerpool
Apr 20, 2023
Merged

[KYUUBI #4713][TEST] Fix false positive result in SchedulerPoolSuite#4714
pan3793 merged 2 commits intoapache:masterfrom
huangzhir:fixtest-schedulerpool

Conversation

@huangzhir
Copy link
Contributor

@huangzhir huangzhir commented Apr 14, 2023 •
edited
Loading

Why are the changes needed?

fix issuse #4713

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@huangzhir
Copy link
Contributor Author

huangzhir commented Apr 15, 2023

Cause of this issue:

  1. threads.shutdown() is asynchronous.
  2. When using eventually to judge, if job2 finishes first and job1 has not finished, the value of job1FinishTime is 0. job2FinishTime - job1FinishTime >= 1000 is definitely greater than 0. The main program will exit the eventually method and end the main thread.
  3. However, job1 has not produced a result yet, and closing the statement and session will cause an exception.
  4. The root cause is that job1 and job2 are executed in multiple threads almost simultaneously, and which one is job1 or job2 is not fixed.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2023 •
edited
Loading

Codecov Report

Merging #4714 (8a753e9) into master (a834ed3) will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4714      +/-   ##
============================================
+ Coverage     57.98%   58.00%   +0.01%     
  Complexity       13       13              
============================================
  Files           580      580              
  Lines         32213    32270      +57     
  Branches       4304     4307       +3     
============================================
+ Hits          18678    18717      +39     
- Misses        11741    11752      +11     
- Partials       1794     1801       +7

see 33 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pan3793 pan3793 changed the title KYUUBI #4713] fix TEST SchedulerPoolSuite a false positive result [KYUUBI #4713][TEST] Fix false positive result in SchedulerPoolSuite Apr 16, 2023
pan3793
pan3793 approved these changes Apr 16, 2023
View reviewed changes
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

Make sense to me, cc @ulysses-you

@pan3793 pan3793 added this to the v1.7.1 milestone Apr 17, 2023
@pan3793 pan3793 closed this in 57b0611 Apr 17, 2023
pan3793 pushed a commit that referenced this pull request Apr 17, 2023
@huangzhir @pan3793
[KYUUBI #4713][TEST] Fix false positive result in SchedulerPoolSuite
84f19a7 
### _Why are the changes needed?_

fix issuse #4713

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [X] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4714 from huangzhir/fixtest-schedulerpool.

Closes #4713

e66ede2 [huangzhir] fixbug TEST SchedulerPoolSuite  a false positive result

Authored-by: huangzhir <306824224@qq.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 57b0611)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Apr 17, 2023

Thanks, merged to master/1.7

@huangzhir
Copy link
Contributor Author

huangzhir commented Apr 17, 2023

hello, @pan3793 There is an error in the code. Can you reopen this PR? This error leads to an obvious failure when checking in the 1.7 branch.

@pan3793 pan3793 reopened this Apr 17, 2023
@huangzhir
Copy link
Contributor Author

huangzhir commented Apr 18, 2023

@pan3793 @ulysses-you I've made some changes to the code,make sure job1 started before job2 and would appreciate it if you could review them.

@pan3793 pan3793 merged commit b4d67e4 into apache:master Apr 20, 2023
pan3793 pushed a commit that referenced this pull request Apr 20, 2023
@huangzhir @pan3793
[KYUUBI #4713][TEST] Fix false positive result in SchedulerPoolSuite (#…
95442ea 
…4714)

### _Why are the changes needed?_

fix issuse #4713


### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [X] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4714 from huangzhir/fixtest-schedulerpool.

Closes #4713

Authored-by: huangzhir <306824224@qq.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Apr 20, 2023

Thanks, merged to master/1.7

@wForget wForget mentioned this pull request Apr 20, 2023
Closed
3 tasks
@huangzhir huangzhir mentioned this pull request Apr 21, 2023
Closed
3 tasks
pan3793 pushed a commit that referenced this pull request Apr 21, 2023
@huangzhir @turboFei @pan3793
[KYUUBI #4749] Fix flaky test issues in SchedulerPoolSuite
2c55a1f 
### _Why are the changes needed?_

To fix issue #4713, a PR  #4714 was submitted, but it had Flaky test issues. After 50 local tests, it succeeded 38 times and failed 12 times.
This PR addresses the issue of flaky tests.

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4749 from huangzhir/fixtest-schedulerpool.

Closes #4749

2d2e140 [huangzhir] call KyuubiSparkContextHelper.waitListenerBus() to make sure there are no more events in the spark event queue
52a34d2 [fwang12] [KYUUBI #4746] Do not recreate async request executor if has been shutdown
d4558ea [huangzhir] Merge branch 'master' into fixtest-schedulerpool
44c4cef [huangzhir] make sure the SparkListener has received the finished events for job1 and job2.
8a753e9 [huangzhir] make sure job1 started before job2
e66ede2 [huangzhir] fixbug TEST SchedulerPoolSuite  a false positive result

Lead-authored-by: huangzhir <306824224@qq.com>
Co-authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 pushed a commit that referenced this pull request Apr 21, 2023
@huangzhir @turboFei @pan3793
[KYUUBI #4749] Fix flaky test issues in SchedulerPoolSuite
26581f4 
### _Why are the changes needed?_

To fix issue #4713, a PR  #4714 was submitted, but it had Flaky test issues. After 50 local tests, it succeeded 38 times and failed 12 times.
This PR addresses the issue of flaky tests.

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4749 from huangzhir/fixtest-schedulerpool.

Closes #4749

2d2e140 [huangzhir] call KyuubiSparkContextHelper.waitListenerBus() to make sure there are no more events in the spark event queue
52a34d2 [fwang12] [KYUUBI #4746] Do not recreate async request executor if has been shutdown
d4558ea [huangzhir] Merge branch 'master' into fixtest-schedulerpool
44c4cef [huangzhir] make sure the SparkListener has received the finished events for job1 and job2.
8a753e9 [huangzhir] make sure job1 started before job2
e66ede2 [huangzhir] fixbug TEST SchedulerPoolSuite  a false positive result

Lead-authored-by: huangzhir <306824224@qq.com>
Co-authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 2c55a1f)
Signed-off-by: Cheng Pan <chengpan@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ulysses-you ulysses-you ulysses-you approved these changes

@pan3793 pan3793 pan3793 approved these changes

+1 more reviewer

@huangtruman huangtruman huangtruman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@huangzhir huangzhir

Labels

module:spark

Projects

None yet

Milestone

v1.7.1

Development

Successfully merging this pull request may close these issues.

5 participants

@huangzhir @codecov-commenter @pan3793 @ulysses-you @huangtruman