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

[WIP][feat]curvefs/client: warmup manager #2234

Merged

Conversation

Cyber-SiKu
Copy link
Contributor

@Cyber-SiKu Cyber-SiKu commented Feb 6, 2023

Signed-off-by: Cyber-SiKu Cyber-SiKu@outlook.com

What problem does this PR solve?

Issue Number: #2039

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@Cyber-SiKu Cyber-SiKu changed the title [feat]curvefs/client: warmup manager [WIP][feat]curvefs/client: warmup manager Feb 6, 2023
@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch 11 times, most recently from 1bc41e5 to ed64f2e Compare February 10, 2023 08:38
@Cyber-SiKu Cyber-SiKu requested review from wu-hanqing, wuhongsong and ilixiaocui and removed request for wu-hanqing, wuhongsong and ilixiaocui February 10, 2023 08:38
@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch 4 times, most recently from 676a3d1 to 70cbc2f Compare February 13, 2023 08:17
@Cyber-SiKu
Copy link
Contributor Author

cicheck

.clang-format Outdated
@@ -1,5 +1,6 @@
---
Language: Cpp
BasedOnStyle: Google
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestions

---
Language: Cpp
BasedOnStyle: Google
AccessModifierOffset: -3
ContinuationIndentWidth: 8
DerivePointerAlignment: false
IndentWidth: 4
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggestions

---
Language: Cpp
BasedOnStyle: Google
AccessModifierOffset: -3
ContinuationIndentWidth: 8
DerivePointerAlignment: false
IndentWidth: 4
...

fix

@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch 3 times, most recently from 89364c6 to dc8f4a6 Compare February 14, 2023 10:56
@Cyber-SiKu
Copy link
Contributor Author

cicheck

@Cyber-SiKu
Copy link
Contributor Author

cichck

@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch 2 times, most recently from ff4e7b2 to fd4e2f3 Compare February 15, 2023 13:21
LOG(ERROR) << "not support warmup type, only support single/list";
ret = ERANGE;
switch (type) {
case curvefs::client::common::WarmupType::kWarmupTypeList:
Copy link
Contributor

Choose a reason for hiding this comment

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

indenet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indenet?

that's ok

@@ -307,9 +325,10 @@ int Warmup(const std::string& name, const std::string& value) {
int ret = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

indent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indent?

fix


void WarmupManagerS3Impl::AddWarmupFilelist(fuse_ino_t key) {
if (!mounted_.load(std::memory_order_acquire)) {
LOG(ERROR) << "not mounted";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe need wait util mounted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe need wait util mounted?

fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe need wait util mounted?

fix


void WarmupManagerS3Impl::AddWarmupFile(fuse_ino_t key) {
if (!mounted_.load(std::memory_order_acquire)) {
LOG(ERROR) << "not mounted";
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

fix

}
// add warmup Progress
if (!AddWarmupProcess(key)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

need logs? and do we need notice the users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

need logs? and do we need notice the users?

Unnecessary, failure indicates that the warm-up task has been joined and not completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

need logs? and do we need notice the users?

fix

if (ret != CURVEFS_ERROR::OK) {
LOG(ERROR) << "inodeManager get inode fail, ret = " << ret
<< ", inodeid = " << key;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe need delete the value added upside?and also do we need notice the users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe need delete the value added upside?and also do we need notice the users?

fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe need delete the value added upside?and also do we need notice the users?

fix

}
// add warmup Progress
if (!AddWarmupProcess(key)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

fix

std::function<void()> task) {
VLOG(9) << "add fetchDentry task: " << key;
std::unique_ptr<ThreadPool> tp = absl::make_unique<ThreadPool>();
tp->Start(warmupThreadsNum_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Each inode has a thread pool. What if there are many files downloaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each inode has a thread pool. What if there are many files downloaded?

A warmup task has a thread pool, and if there are many files, share the thread pool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each inode has a thread pool. What if there are many files downloaded?

A warmup task has a thread pool, and if there are many files, share the thread pool

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, need limit the task nums(thread nums)

Copy link
Contributor

Choose a reason for hiding this comment

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

Each inode has a thread pool. What if there are many files downloaded?

A warmup task has a thread pool, and if there are many files, share the thread pool

threads will be created even same key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

threads will be created even same key

that is todo work

@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch 7 times, most recently from dcae9c7 to 126803f Compare February 16, 2023 13:47
1. add WarmupManager
2. add WarmupManagerS3Impl
3. add query warmup progress in tools-v2

Signed-off-by: Cyber-SiKu <Cyber-SiKu@outlook.com>
@Cyber-SiKu Cyber-SiKu force-pushed the curvefs/client_warmup_manager branch from ffef139 to caa5677 Compare February 17, 2023 05:04
@Cyber-SiKu
Copy link
Contributor Author

cicheck

@wuhongsong wuhongsong merged commit e03e921 into opencurve:master Feb 20, 2023
@Cyber-SiKu Cyber-SiKu deleted the curvefs/client_warmup_manager branch February 21, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants