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

Fix storage hang on terminate #3014

Merged

Conversation

liwenhui-soul
Copy link
Contributor

No description provided.

@liwenhui-soul liwenhui-soul added the ready-for-testing PR: ready for the CI test label Oct 8, 2021
@liwenhui-soul liwenhui-soul force-pushed the fix-storage-hang-on-terminate branch 2 times, most recently from 90d02b3 to aa76a96 Compare October 8, 2021 17:24
@liwenhui-soul liwenhui-soul force-pushed the fix-storage-hang-on-terminate branch from aa76a96 to ee0517a Compare October 9, 2021 05:43
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Oct 9, 2021
@liwenhui-soul liwenhui-soul added the cherry-pick-v2.6 PR: need cherry-pick to this version label Oct 9, 2021
Copy link
Contributor

@liuyu85cn liuyu85cn left a comment

Choose a reason for hiding this comment

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

general looks good to me, plz note some code style

src/storage/admin/AdminTaskManager.cpp Outdated Show resolved Hide resolved
src/storage/admin/AdminTaskManager.cpp Outdated Show resolved Hide resolved
src/storage/admin/AdminTaskManager.cpp Outdated Show resolved Hide resolved
src/storage/admin/AdminTaskManager.h Outdated Show resolved Hide resolved
@liwenhui-soul liwenhui-soul force-pushed the fix-storage-hang-on-terminate branch 2 times, most recently from f05e215 to a7327e5 Compare October 9, 2021 06:51
critical27
critical27 previously approved these changes Oct 9, 2021
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

LGTM

@critical27
Copy link
Contributor

Close #3005

@liwenhui-soul liwenhui-soul force-pushed the fix-storage-hang-on-terminate branch 2 times, most recently from 15465a8 to 1d10dcd Compare October 9, 2021 14:41
@codecov-commenter
Copy link

Codecov Report

Merging #3014 (04aa4df) into master (e642c05) will increase coverage by 0.01%.
The diff coverage is 33.33%.

❗ Current head 04aa4df differs from pull request most recent head 1d10dcd. Consider uploading reports for the commit 1d10dcd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3014      +/-   ##
==========================================
+ Coverage   84.34%   84.36%   +0.01%     
==========================================
  Files        1283     1283              
  Lines      113684   113699      +15     
==========================================
+ Hits        95891    95923      +32     
+ Misses      17793    17776      -17     
Impacted Files Coverage Δ
src/storage/admin/AdminTaskManager.h 100.00% <ø> (ø)
src/storage/admin/AdminTaskProcessor.cpp 0.00% <0.00%> (ø)
src/storage/admin/AdminTaskManager.cpp 49.55% <34.28%> (-1.18%) ⬇️
src/kvstore/raftex/Host.cpp 68.60% <0.00%> (-6.40%) ⬇️
src/kvstore/wal/AtomicLogBuffer.h 96.72% <0.00%> (-0.55%) ⬇️
src/common/datatypes/ValueOps-inl.h 60.32% <0.00%> (-0.21%) ⬇️
src/kvstore/NebulaStore.cpp 68.73% <0.00%> (-0.14%) ⬇️
src/graph/util/SchemaUtil.cpp 92.17% <0.00%> (-0.04%) ⬇️
src/common/datatypes/Value.cpp 73.36% <0.00%> (+0.07%) ⬆️
src/common/expression/Expression.cpp 45.92% <0.00%> (+0.08%) ⬆️
... and 9 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 e642c05...1d10dcd. Read the comment docs.

@liwenhui-soul liwenhui-soul force-pushed the fix-storage-hang-on-terminate branch from 1d10dcd to 00d53f3 Compare October 11, 2021 03:57
Copy link
Contributor

@liuyu85cn liuyu85cn left a comment

Choose a reason for hiding this comment

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

LGTM

std::vector<std::string> keys;
std::vector<futTuple> futVec;
for (; iter->valid(); iter->next()) {
folly::StringPiece key = iter->key();
int32_t seqId = *reinterpret_cast<const int32_t*>(key.data());
if (seqId < 0) continue;
if (seqId < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the key is consists of several items. would check it cast to integer?

@@ -33,6 +33,7 @@ include_directories(AFTER ${CMAKE_CURRENT_BINARY_DIR}/src)

if(ENABLE_WERROR)
add_compile_options(-Werror)
add_compile_options(-Wno-attributes)
Copy link
Contributor

@darionyaphet darionyaphet Oct 11, 2021

Choose a reason for hiding this comment

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

why support this compile flag? which problem dose it solved?

@critical27 critical27 merged commit 64721f1 into vesoft-inc:master Oct 11, 2021
Sophie-Xie pushed a commit that referenced this pull request Oct 12, 2021
* add -Wno-attribute, to avoid gcc's bug

* break loop in unreportedAdminThread_ if the sever being stopped
@kikimo kikimo mentioned this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-v2.6 PR: need cherry-pick to this version ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants