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

move balancer to jobManager #3254

Merged
merged 2 commits into from
Nov 17, 2021
Merged

move balancer to jobManager #3254

merged 2 commits into from
Nov 17, 2021

Conversation

liwenhui-soul
Copy link
Contributor

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

What type of PR is this?

  • bug
  • feature

Which issue(s) this PR fixes:

close #xxx
(If it is requirement, issue(s) number must be listed.)

What this PR does / why we need it?

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

Additional context:

Checklist:

  • Documentation affected (If need to modify document, please label it.)
  • Incompatible (If it is incompatile, please describle it and label it.)
  • Need to cherry pick (If need to cherry pick to some branchs, please label the destination version(s).)
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory

Release notes:

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

                                                            `

src/interface/meta.thrift Show resolved Hide resolved
src/kvstore/KVEngine.h Outdated Show resolved Hide resolved
src/meta/processors/job/JobDescription.cpp Show resolved Hide resolved
src/meta/processors/job/BalanceJobExecutor.h Show resolved Hide resolved
src/meta/processors/job/BalancePlan.cpp Outdated Show resolved Hide resolved
@critical27 critical27 added doc affected PR: improvements or additions to documentation incompatible PR: incompatible with the recently released version labels Nov 12, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2021

Codecov Report

Merging #3254 (7357c90) into master (fc2977b) will increase coverage by 0.12%.
The diff coverage is 73.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   85.24%   85.37%   +0.12%     
==========================================
  Files        1305     1287      -18     
  Lines      120347   119667     -680     
==========================================
- Hits       102595   102168     -427     
+ Misses      17752    17499     -253     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.cpp 76.88% <ø> (+1.58%) ⬆️
src/clients/meta/MetaClient.h 95.65% <ø> (ø)
src/clients/storage/StorageClientBase.h 65.57% <ø> (ø)
src/graph/executor/Executor.cpp 86.58% <ø> (+2.56%) ⬆️
src/graph/executor/admin/SubmitJobExecutor.h 100.00% <ø> (ø)
src/graph/optimizer/rule/IndexScanRule.h 100.00% <ø> (ø)
src/graph/planner/plan/Admin.cpp 58.18% <ø> (+3.94%) ⬆️
src/graph/planner/plan/Admin.h 73.23% <ø> (+4.45%) ⬆️
src/graph/planner/plan/PlanNode.cpp 85.42% <ø> (+2.37%) ⬆️
src/graph/planner/plan/PlanNode.h 81.25% <ø> (ø)
... and 79 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 7f711bb...7357c90. Read the comment docs.

@critical27
Copy link
Contributor

Good job

processors/admin/Balancer.cpp
processors/admin/BalancePlan.cpp
processors/admin/BalanceTask.cpp
processors/job/BalancePlan.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

fix indent?

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.

Awesome! LGTM

@@ -9,11 +9,11 @@ set(expression_test_common_libs
$<TARGET_OBJECTS:network_obj>
$<TARGET_OBJECTS:fs_obj>
$<TARGET_OBJECTS:stats_obj>
$<TARGET_OBJECTS:datatypes_obj>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask the purpose of this order change?

@critical27
Copy link
Contributor

both balance data and balance leader will only work for one space, so use space at first

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-testing PR: ready for the CI test
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants