Skip to content

meta client thread local access. #2165

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

Merged
merged 4 commits into from
Jun 15, 2020

Conversation

xuguruogu
Copy link
Collaborator

use thread local object when access meta client cache. folly::SingletonThreadLocal supply efficient access wrapper by using pointer.

@xuguruogu xuguruogu force-pushed the meta-client-thread-local branch from be98763 to 498b5cd Compare June 5, 2020 11:49
@xuguruogu
Copy link
Collaborator Author

the issue came from compaction.

before the changes:
image

after the changes:
image

@dutor dutor added the ready-for-testing PR: ready for the CI test label Jun 8, 2020
@dutor
Copy link
Contributor

dutor commented Jun 8, 2020

Thanks for this improvement. I also noticed this issue recently, and did some similar optimization. Many other modules may benefit from this patch.

dangleptr
dangleptr previously approved these changes Jun 8, 2020
Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for your contribution!

@dangleptr
Copy link
Contributor

Please recheck the Failed UTs

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #2165 into master will decrease coverage by 0.00%.
The diff coverage is 91.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2165      +/-   ##
==========================================
- Coverage   86.40%   86.39%   -0.01%     
==========================================
  Files         643      643              
  Lines       62686    62722      +36     
==========================================
+ Hits        54161    54187      +26     
- Misses       8525     8535      +10     
Impacted Files Coverage Δ
src/kvstore/NebulaStore.cpp 82.08% <0.00%> (-0.29%) ⬇️
src/meta/client/MetaClient.h 100.00% <ø> (ø)
src/meta/client/MetaClient.cpp 81.20% <92.95%> (+0.41%) ⬆️
src/kvstore/raftex/Host.cpp 57.95% <0.00%> (-3.31%) ⬇️
src/storage/admin/AdminProcessor.h 70.29% <0.00%> (-1.68%) ⬇️
src/common/fs/test/FileUtilsTest.cpp 99.47% <0.00%> (-0.53%) ⬇️
src/meta/processors/admin/AdminClient.cpp 80.18% <0.00%> (-0.46%) ⬇️
src/kvstore/test/NebulaStoreTest.cpp 97.48% <0.00%> (ø)
src/dataman/RowReader.cpp 77.98% <0.00%> (+0.31%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c199198...b66f158. Read the comment docs.

@xuguruogu xuguruogu requested a review from dangleptr June 8, 2020 07:21
@xuguruogu xuguruogu force-pushed the meta-client-thread-local branch from 52e2391 to bfe67fc Compare June 8, 2020 07:27
@xuguruogu
Copy link
Collaborator Author

centos7 unit tests is killed. may due to make oom

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

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

LGTM.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@dangleptr dangleptr merged commit f1cfa0f into vesoft-inc:master Jun 15, 2020
@xuguruogu xuguruogu deleted the meta-client-thread-local branch June 15, 2020 09:32
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
…c op. (vesoft-inc#2165)

Co-authored-by: trippli <trippli@tencent.com>
Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants