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

Balance #3394

Merged
merged 7 commits into from
Dec 29, 2021
Merged

Balance #3394

merged 7 commits into from
Dec 29, 2021

Conversation

liwenhui-soul
Copy link
Contributor

@liwenhui-soul liwenhui-soul commented Dec 2, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

Which issue(s)/PR(s) this PR relates to?

#3169
#3432

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
    https://confluence.nebula-graph.io/pages/viewpage.action?pageId=10729589
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@liwenhui-soul liwenhui-soul added the ready-for-testing PR: ready for the CI test label Dec 2, 2021
@liwenhui-soul liwenhui-soul force-pushed the balance branch 3 times, most recently from 0970fe0 to 8ef5d52 Compare December 8, 2021 12:19
@liwenhui-soul liwenhui-soul added the do not review PR: not ready for the code review yet label Dec 10, 2021
@liwenhui-soul liwenhui-soul removed the do not review PR: not ready for the code review yet label Dec 10, 2021
@liwenhui-soul liwenhui-soul force-pushed the balance branch 4 times, most recently from f6cc878 to a52fb46 Compare December 14, 2021 09:00
@liwenhui-soul liwenhui-soul mentioned this pull request Dec 14, 2021
7 tasks
@wenhaocs
Copy link
Contributor

Can you put description of this PR at the beginning?
As a general rule, we may put:

  • Problem
  • Steps to solve this problem
  • What this PR achieves (in the steps above)

A clear description is as important as code.

Thank you

@liwenhui-soul liwenhui-soul force-pushed the balance branch 2 times, most recently from 89f99bf to 242d10d Compare December 15, 2021 04:07
@liwenhui-soul liwenhui-soul added doc affected PR: improvements or additions to documentation incompatible PR: incompatible with the recently released version labels Dec 15, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2021

Codecov Report

Merging #3394 (9277195) into master (99f1f7a) will decrease coverage by 0.06%.
The diff coverage is 81.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3394      +/-   ##
==========================================
- Coverage   85.19%   85.13%   -0.07%     
==========================================
  Files        1304     1315      +11     
  Lines      121308   121302       -6     
==========================================
- Hits       103352   103265      -87     
- Misses      17956    18037      +81     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 95.65% <ø> (ø)
src/clients/storage/StorageClient.h 100.00% <ø> (ø)
src/clients/storage/StorageClientBase-inl.h 74.09% <ø> (+1.41%) ⬆️
src/graph/executor/Executor.cpp 83.23% <0.00%> (-0.51%) ⬇️
src/graph/executor/admin/SpaceExecutor.cpp 80.57% <0.00%> (-5.94%) ⬇️
src/graph/executor/admin/SpaceExecutor.h 85.71% <0.00%> (-14.29%) ⬇️
src/graph/planner/plan/Admin.h 65.01% <0.00%> (-1.66%) ⬇️
src/graph/planner/plan/PlanNode.cpp 82.30% <0.00%> (-0.47%) ⬇️
src/graph/planner/plan/PlanNode.h 81.25% <ø> (ø)
src/graph/service/PermissionCheck.cpp 78.72% <ø> (ø)
... and 85 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 e6ddeb2...9277195. Read the comment docs.

Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

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

Half is done so far. Could you add more essential comments across files to help facilitate review and future reading from peers? Thank you!

src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
sortedHosts.front()->parts_.emplace(partId);
zone->partNum_++;
HostAddr ha = sortedHosts.front()->ha_;
for (size_t i = 0; i < sortedHosts.size() - 1; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you loop the sorted hosts and find the first host which meets:
sortedHosts[i]->parts_.size() < sortedHosts[i + 1]
to avoid swap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't you loop the sorted hosts and find the first host which meets: sortedHosts[i]->parts_.size() < sortedHosts[i + 1] to avoid swap?

swap pointers is not performance sensitive and it's easy to understand

for (; newLeftIndex < leftEnd - 1; newLeftIndex++) {
if (sortedActiveZones[newLeftIndex]->partNum_ >
sortedActiveZones[newLeftIndex + 1]->partNum_) {
std::swap(sortedActiveZones[newLeftIndex], sortedActiveZones[newLeftIndex + 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

the same, why not pick the right zone to insert to avoid swap?

src/meta/processors/job/BalanceJobExecutor.cpp Outdated Show resolved Hide resolved
critical27
critical27 previously approved these changes Dec 24, 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 now, great work~

wenhaocs
wenhaocs previously approved these changes Dec 24, 2021
critical27
critical27 previously approved these changes Dec 27, 2021
critical27
critical27 previously approved these changes Dec 28, 2021
src/meta/processors/job/BalanceJobExecutor.h Show resolved Hide resolved
src/meta/processors/job/BalancePlan.cpp Show resolved Hide resolved
nebula::kvstore::KVStore* kvStore_{nullptr};
AdminClient* adminClient_{nullptr};

std::mutex muReportFinish_;
std::mutex muJobFinished_;
std::atomic<JbmgrStatus> status_ = JbmgrStatus::NOT_START;
Copy link
Contributor

Choose a reason for hiding this comment

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

good job


/* first, move the lostZones' parts to the active zones
* second, make balance for the active zones */
Status ZoneBalanceJobExecutor::buildBalancePlan() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see comments added!

Copy link
Contributor

@pengweisong pengweisong left a comment

Choose a reason for hiding this comment

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

LGTM

@CPWstatic CPWstatic merged commit 62d8787 into vesoft-inc:master Dec 29, 2021
@cooper-lzy cooper-lzy self-requested a review January 5, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation incompatible PR: incompatible with the recently released version ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants