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

[BugFix] should acquire db lock out of createLoadTask to avoid dead lock #55219

Merged
merged 3 commits into from
Jan 20, 2025
Merged

[BugFix] should acquire db lock out of createLoadTask to avoid dead lock #55219

merged 3 commits into from
Jan 20, 2025

Conversation

kaijianding
Copy link
Contributor

Why I'm doing:

A dead lock issue is found:

Slow lock:
image

Thread "pool-21-thread-406" owns the database lock.
StreamLoadMgr.cleanSyncStreamLoadTasks() tries to acquire StreamLoadMgr.lock.writeLock() 0x00007fb9905311a8

Jstack:
image

Thread "thrift-server-pool-6208" owns the StreamLoadMgr.lock.writeLock(): 0x00007fb9905311a8 in StreamLoadMgr.beginLoadTask()

StreamLoadMgr.beginLoadTask() call StreamLoadMgr.createLoadTask() who is trying to acquire database lock

What I'm doing:

StreamLoadMgr.createLoadTask() don't acquire database lock.
StreamLoadMgr.beginLoadTask() first acquire database lock and unlock, then acquire StreamLoadMgr.lock.writeLock()

Fixes #issue

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@kaijianding
Copy link
Contributor Author

@smartlxh becuase u r the owner of #21441, could you take a look at this pr?

@kevincai
Copy link
Contributor

The database lock is held outside the StreamLoadMgr and then call StreamLoadMgr with acquiring its write lock. StreamLoadMgr can't be aware of the database lock the caller holds.

So the best practice of the StreamLoadMgr implementation is, DON'T TRY TO ACQUIRE DB/TABLE READ/WRITE LOCK under StreamLoadMgr's read/write lock.

@kaijianding
use private
5715d73 
Signed-off-by: kaijian.ding <[email protected]>
@kaijianding
rename
22e17dc 
Signed-off-by: kaijian.ding <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@wyb wyb enabled auto-merge (squash) January 20, 2025 08:48
@github-actions GitHub Actions
Copy link

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions GitHub Actions
Copy link

[FE Incremental Coverage Report]

✅ pass : 9 / 9 (100.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/load/routineload/RoutineLoadJob.java 1 1 100.00% []
🔵 com/starrocks/load/streamload/StreamLoadMgr.java 8 8 100.00% []

@github-actions GitHub Actions
Copy link

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@wyb wyb merged commit 9af1024 into StarRocks:main Jan 20, 2025
48 of 49 checks passed
@github-actions GitHub Actions
Copy link

@Mergifyio backport branch-3.4

@github-actions github-actions bot removed the 3.4 label Jan 20, 2025
@github-actions GitHub Actions
Copy link

@Mergifyio backport branch-3.3

@github-actions github-actions bot removed the 3.3 label Jan 20, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 20, 2025 •
edited
Loading

backport branch-3.4

✅ Backports have been created

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 20, 2025 •
edited
Loading

backport branch-3.3

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 20, 2025
@kaijianding @mergify
[BugFix] should acquire db lock out of createLoadTask to avoid dead l…
66c4a0c 
…ock (#55219)

Signed-off-by: kaijian.ding <[email protected]>
(cherry picked from commit 9af1024)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/load/streamload/StreamLoadMgr.java
@mergify mergify bot mentioned this pull request Jan 20, 2025
Merged
10 tasks
mergify bot pushed a commit that referenced this pull request Jan 20, 2025
@kaijianding @mergify
[BugFix] should acquire db lock out of createLoadTask to avoid dead l…
29dda45 
…ock (#55219)

Signed-off-by: kaijian.ding <[email protected]>
(cherry picked from commit 9af1024)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/load/streamload/StreamLoadMgr.java
@mergify mergify bot mentioned this pull request Jan 20, 2025
Merged
10 tasks
@kaijianding kaijianding deleted the db_lock branch January 20, 2025 10:02
wanpengfei-git pushed a commit that referenced this pull request Jan 22, 2025
@mergify @kaijianding @kevincai
[BugFix] should acquire db lock out of createLoadTask to avoid dead l…
27a9987 
…ock (backport #55219) (#55263)

Signed-off-by: Kevin Xiaohua Cai <[email protected]>
Co-authored-by: kaijianding <[email protected]>
Co-authored-by: Kevin Xiaohua Cai <[email protected]>
wanpengfei-git pushed a commit that referenced this pull request Jan 22, 2025
@mergify @kaijianding @kevincai
[BugFix] should acquire db lock out of createLoadTask to avoid dead l…
0467d48 
…ock (backport #55219) (#55264)

Signed-off-by: Kevin Xiaohua Cai <[email protected]>
Co-authored-by: kaijianding <[email protected]>
Co-authored-by: Kevin Xiaohua Cai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wyb wyb wyb approved these changes

@kevincai kevincai kevincai approved these changes

Assignees

@kaijianding kaijianding

Labels
3.3-merged 3.4-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaijianding @kevincai @wyb