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

Improve memory watermark detection #2885

Merged
merged 34 commits into from
Oct 8, 2021

Conversation

yixinglu
Copy link
Contributor

fix #2497 and fix #2591

@yixinglu yixinglu added the wip Solution: work in progress label Sep 16, 2021
@yixinglu yixinglu force-pushed the mem-watermark branch 3 times, most recently from 4569886 to 2f225fd Compare September 22, 2021 11:27
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 27, 2021
@yixinglu yixinglu force-pushed the mem-watermark branch 4 times, most recently from fb00cee to cb4b57e Compare September 29, 2021 02:21
@yixinglu yixinglu added ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Sep 30, 2021
@HarrisChu
Copy link
Contributor

HarrisChu commented Sep 30, 2021

cc @MegaByte875

Watermark would check /sys/fs/cgroup/memory.limit_in_bytes in container.
Means if the pod limit is 2G, it would prevent queries if the memory is 1.6G by default.

czpmango
czpmango previously approved these changes Sep 30, 2021
Copy link
Contributor

@czpmango czpmango left a comment

Choose a reason for hiding this comment

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

LGTM.

instance->execute();
}

Status QueryEngine::setupBackgroundThread() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We had many background thread now, better name it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Such as MemoryMonitor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I think we could use the same thread to do the background task in the future.

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.

Generally LGTM, wait conflicts resolved.

Comment on lines +28 to +30
throw std::runtime_error(
folly::sformat("Used memory hits the high watermark({}) of total system memory.",
FLAGS_system_memory_high_watermark_ratio));
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is handler the runtime error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is handler the runtime error?

What's about as below ?

if (MemoryUtils::kHitMemoryHighWatermark.load()) {
LOG(ERROR);
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a thenError in QueryInstance

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a thenError in QueryInstance

got . LGTM now. Please resolved conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stop the query execution by throwing the exception rather than skip some rows.

@codecov-commenter
Copy link

Codecov Report

Merging #2885 (ba3d92a) into master (9462d35) will decrease coverage by 0.09%.
The diff coverage is 89.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2885      +/-   ##
==========================================
- Coverage   84.40%   84.30%   -0.10%     
==========================================
  Files        1284     1283       -1     
  Lines      113608   113679      +71     
==========================================
- Hits        95888    95839      -49     
- Misses      17720    17840     +120     
Impacted Files Coverage Δ
src/clients/storage/StorageClientBase-inl.h 78.57% <0.00%> (-4.17%) ⬇️
src/common/thrift/ThriftClientManager.h 100.00% <ø> (ø)
src/daemons/GraphDaemon.cpp 65.13% <ø> (ø)
src/graph/executor/Executor.h 100.00% <ø> (ø)
src/graph/executor/query/GetPropExecutor.h 70.58% <ø> (-1.64%) ⬇️
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (-0.31%) ⬇️
src/graph/service/GraphService.h 100.00% <ø> (ø)
src/graph/service/QueryEngine.h 100.00% <ø> (ø)
src/graph/util/Utils.h 100.00% <ø> (ø)
src/graph/executor/query/FilterExecutor.cpp 85.71% <25.00%> (-3.58%) ⬇️
... and 33 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 9462d35...ba3d92a. Read the comment docs.

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.

Good job

@critical27 critical27 merged commit e642c05 into vesoft-inc:master Oct 8, 2021
@yixinglu yixinglu deleted the mem-watermark branch October 8, 2021 01:59
@Sophie-Xie Sophie-Xie added doc affected PR: improvements or additions to documentation and removed doc affected PR: improvements or additions to documentation labels Oct 19, 2021
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.

[QE]Memory usage threshold refinery memory_high_watermark use freeram as available memory
8 participants