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.

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

Add operation lock instead of locking state Enumeration #4739

Closed
wants to merge 4 commits into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Apr 20, 2023

Why are the changes needed?

We meet an issue that cause all the operation stuck when closing operation.

Because now all the operations try to lock a Scala Enumeration val.

And if one of them stuck, all the others will be keep stuck.

In this pr, I add a lock for each operation.

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

@turboFei turboFei changed the title op lock Add operation lock instead of locking state Enumeration Apr 20, 2023
@turboFei
Copy link
Member Author

cc @yaooqinn @pan3793 @ulysses-you

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #4739 (535400a) into master (17514a3) will decrease coverage by 0.01%.
The diff coverage is 80.95%.

@@             Coverage Diff              @@
##             master    #4739      +/-   ##
============================================
- Coverage     58.03%   58.02%   -0.01%     
  Complexity       13       13              
============================================
  Files           580      581       +1     
  Lines         32270    32320      +50     
  Branches       4307     4310       +3     
============================================
+ Hits          18728    18754      +26     
- Misses        11739    11761      +22     
- Partials       1803     1805       +2     
Impacted Files Coverage Δ
.../kyuubi/engine/spark/operation/ExecutePython.scala 0.00% <0.00%> (ø)
...g/apache/kyuubi/operation/BatchJobSubmission.scala 75.00% <50.00%> (-0.67%) ⬇️
...kyuubi/engine/spark/operation/SparkOperation.scala 75.65% <66.66%> (-0.66%) ⬇️
...rg/apache/kyuubi/operation/AbstractOperation.scala 83.33% <66.66%> (+0.37%) ⬆️
...he/kyuubi/engine/spark/repl/KyuubiSparkILoop.scala 93.84% <100.00%> (-0.19%) ⬇️
...ommon/src/main/scala/org/apache/kyuubi/Utils.scala 71.51% <100.00%> (+0.50%) ⬆️
.../apache/kyuubi/client/KyuubiSyncThriftClient.scala 88.30% <100.00%> (-0.05%) ⬇️
.../org/apache/kyuubi/operation/KyuubiOperation.scala 73.91% <100.00%> (-2.50%) ⬇️

... and 20 files with indirect coverage changes

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

@turboFei turboFei self-assigned this Apr 20, 2023
@turboFei turboFei added this to the v1.7.1 milestone Apr 20, 2023
@turboFei turboFei requested a review from yaooqinn April 20, 2023 12:15
Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

LGTM

@pan3793
Copy link
Member

pan3793 commented Apr 21, 2023

ResourceLock is definitely a future-proof option

trinodb/trino#17045

@turboFei turboFei closed this in ccacb33 Apr 21, 2023
@turboFei
Copy link
Member Author

thanks, merged to master

there is conflicts with branch-1.7,

pan3793 pushed a commit that referenced this pull request Apr 21, 2023
We meet an issue that cause all the operation stuck when closing operation.

Because now all the operations try to lock a Scala Enumeration val.

And if one of them stuck, all the others will be keep stuck.

In this pr, I add a lock for each operation.
- [ ] 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 #4739 from turboFei/op_lock.

Closes #4739

535400a [fwang12] revert
a934389 [fwang12] lockInterruptibly
274abc9 [fwang12] utils
ceda731 [fwang12] op lock

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
@pan3793
Copy link
Member

pan3793 commented Apr 21, 2023

Cherry-picked to 1.7

beryllw pushed a commit to beryllw/incubator-kyuubi that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants