Skip to content

[enhancement](cloud) optimize block cache lock - #41818

Merged
gavinchou merged 5 commits into
apache:masterfrom
freemandealer:opt-cache-lock
Oct 30, 2024
Merged

[enhancement](cloud) optimize block cache lock#41818
gavinchou merged 5 commits into
apache:masterfrom
freemandealer:opt-cache-lock

Conversation

@freemandealer

@freemandealerfreemandealer commented Oct 14, 2024

Copy link
Copy Markdown
Member
  1. async deletion when do stale rowsets reclycle
  2. minimize lock critical size
  3. add cache lock held & wait time info for debug

1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
@doris-robot

Copy link
Copy Markdown

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -19,6 +19,7 @@

#include <bvar/bvar.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: 'bvar/bvar.h' file not found [clang-diagnostic-error]

#include<bvar/bvar.h>
^

Comment threadbe/src/io/cache/block_file_cache.h Outdated

class LockScopedTimer {
public:
LockScopedTimer() : start_(std::chrono::high_resolution_clock::now()) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

Suggested change
LockScopedTimer() : start_(std::chrono::high_resolution_clock::now()) {}
LockScopedTimer() : start_(std::chrono::high_resolution_clock::now()) =default;

Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
@freemandealer

Copy link
Copy Markdown
MemberAuthor

run buildall

1 similar comment
@freemandealer

Copy link
Copy Markdown
MemberAuthor

run buildall

Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>
@freemandealer

Copy link
Copy Markdown
MemberAuthor

run buildall

Signed-off-by: freemandealer <freeman.zhang1992@gmail.com>

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


class LockScopedTimer {
public:
LockScopedTimer() : start_(std::chrono::steady_clock::now()) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

Suggested change
LockScopedTimer() : start_(std::chrono::steady_clock::now()) {}
LockScopedTimer() : start_(std::chrono::steady_clock::now()) =default;

@freemandealer

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 37.45% (9709/25922)
Line Coverage: 28.73% (80607/280562)
Region Coverage: 28.19% (41725/148027)
Branch Coverage: 24.76% (21212/85660)
Coverage Report: http://coverage.selectdb-in.cc/coverage/35aac9ef8aa51fa6981f8d4d2c6c9bd1c82b396e_35aac9ef8aa51fa6981f8d4d2c6c9bd1c82b396e/report/index.html

std::chrono::duration_cast<std::chrono::milliseconds>(acq_time - start_time).count(); \
if (duration > config::cache_lock_long_tail_threshold) \
LOG(WARNING) << "Lock wait time " << std::to_string(duration) << "ms. " \
<< get_stack_trace_by_boost() << std::endl; \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should limit frequency of the log.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@dataroaring It is limited already: only > config::cache_lock_long_tail_threshold (default:1s) will print the log. If not enough, enlarge the threshold.

@freemandealer

Copy link
Copy Markdown
MemberAuthor

run cloud_p0

@freemandealer

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 37.46% (9712/25925)
Line Coverage: 28.72% (80628/280694)
Region Coverage: 28.18% (41753/148155)
Branch Coverage: 24.74% (21211/85726)
Coverage Report: http://coverage.selectdb-in.cc/coverage/00ae5bd2f3bd4c4cfb87018fc0a23d5a733af054_00ae5bd2f3bd4c4cfb87018fc0a23d5a733af054/report/index.html

Comment threadbe/src/io/cache/block_file_cache.h
Comment threadbe/src/common/config.h
while (_recycle_keys->pop(key)) {
Status st = _storage->remove(key);
if (!st.ok()) {
LOG_WARNING("").error(st);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

put some key word in the log e.g.
"failed to remove when recycle_stale_rowset_async_bottom_half, key = {}" ...

LOG_WARNING("Failed to push recycle key to queue, do it synchronously");
Status st = _storage->remove(key);
if (!st.ok()) {
LOG_WARNING("").error(st);

@gavinchougavinchouOct 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

put some key word in the log e.g.
"failed to remove async, key = {}" ...

@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Oct 30, 2024
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@HastyshellHastyshell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@gavinchou
gavinchou merged commit cf1714c into apache:masterOct 30, 2024
github-actionsBot pushed a commit that referenced this pull request Oct 30, 2024
1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
freemandealer added a commit to freemandealer/doris that referenced this pull request Nov 7, 2024
pick apache#41818 from master
1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
dataroaring pushed a commit that referenced this pull request Nov 7, 2024
pick #41818 from master
1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
eldenmoon pushed a commit to eldenmoon/incubator-doris that referenced this pull request Nov 12, 2024
…#43401)
pick apache#41818 from master
1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
@gavinchougavinchou mentioned this pull request Nov 26, 2024
HappenLee pushed a commit to HappenLee/incubator-doris that referenced this pull request Apr 24, 2026
1. async deletion when do stale rowsets reclycle
2. minimize lock critical size
3. add cache lock held & wait time info for debug
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by one committer.dev/3.0.3-mergedreviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@freemandealer@doris-robot@gavinchou@Hastyshell@dataroaring