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

refactor(storage): replace RwLocks with DashMap #8639

Merged
merged 6 commits into from
Nov 6, 2022
Merged

refactor(storage): replace RwLocks with DashMap #8639

merged 6 commits into from
Nov 6, 2022

Conversation

ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented Nov 4, 2022 •
edited
Loading

Signed-off-by: 蔡略 cailue@bupt.edu.cn

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

This PR replaced RwLock with dashmap::DashMap, in storages and service

Part of: #8601

@ClSlaid
refactor(storage, service): replace RwLocks with DashMap
5f8ac7d 
Signed-off-by: 蔡略 <cailue@bupt.edu.cn>
@vercel
Copy link

vercel bot commented Nov 4, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Nov 6, 2022 at 6:06PM (UTC)

@mergify mergify bot added the pr-refactor this PR changes the code base without new features or bugfix label Nov 4, 2022
@ClSlaid ClSlaid requested a review from xudong963 November 4, 2022 09:49
@Xuanwo
Copy link
Member

Xuanwo commented Nov 4, 2022

jorgecarleitao/arrow2#1287 has been merged. Please update the arrow2 commits to upstream instead.

@ClSlaid
refactor: update arrow2 upstream
1de9351 
Signed-off-by: ClSlaid <cailue@bupt.edu.cn>
@BohuTANG
Copy link
Member

BohuTANG commented Nov 5, 2022

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2022

update

✅ Branch has been successfully updated

@BohuTANG
Copy link
Member

BohuTANG commented Nov 5, 2022

Does this PR break the max_active_sessions related logic?

There are many errors:

Runner: clickhouse
ErrorType: statement query execute with exception
Message: Failed to execute. Collected info: Orig exception: Code: 1041, displayText = The current accept connection has exceeded max_active_sessions config(while in query flight).

@ClSlaid ClSlaid marked this pull request as draft November 5, 2022 08:35
@ClSlaid
fix: try fix with AcqRel
301e53d 
Signed-off-by: 蔡略 <cailue@bupt.edu.cn>
@ClSlaid
sometimes Arc<RwLock> is good enough
06cb89e 
Signed-off-by: 蔡略 <cailue@bupt.edu.cn>
@ClSlaid
Copy link
Contributor Author

ClSlaid commented Nov 6, 2022

I have to admit refactoring the SessionManager with DashMap is a bad idea. It's hard to get done right, and It's not that necessary since it is hardly a heat point of performance.

@ClSlaid ClSlaid changed the title refactor(storage, service): replace RwLocks with DashMap refactor(storage): replace RwLocks with DashMap Nov 6, 2022
PsiACE
PsiACE approved these changes Nov 6, 2022
View reviewed changes
Copy link
Member

@PsiACE PsiACE left a comment

Choose a reason for hiding this comment

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

lgtm

@Xuanwo Xuanwo marked this pull request as ready for review November 6, 2022 18:04
@Xuanwo
Copy link
Member

Xuanwo commented Nov 6, 2022 •
edited
Loading

Please set up ready for review if you are ready 🤣

@mergify mergify bot merged commit 460631a into databendlabs:main Nov 6, 2022
@ClSlaid ClSlaid deleted the replace-rwlock-storages-service branch November 7, 2022 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PsiACE PsiACE PsiACE approved these changes

@Xuanwo Xuanwo Xuanwo approved these changes

@BohuTANG BohuTANG BohuTANG approved these changes

@xudong963 xudong963 Awaiting requested review from xudong963

Assignees
No one assigned
Labels
pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ClSlaid @Xuanwo @BohuTANG @PsiACE