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.

Sign up for GitHub

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

Jump to bottom

planner, add hint nth_plan(x) to help user force a plan #17850

Merged
merged 52 commits into from
Jul 1, 2020
Merged

planner, add hint nth_plan(x) to help user force a plan #17850

merged 52 commits into from
Jul 1, 2020

Conversation

danmay319
Copy link
Contributor

@danmay319 danmay319 commented Jun 8, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: to #17580

Problem Summary:

  • add return param cntPlan for find_best_task to indicate the number of plans found in it.
  • for more see below.

What is changed and how it works?

Proposal: plan test

What's Changed:

How it Works:

  • statistics the number of found plans in find_best_task().
  • enable countdown in function findBestTask, implement type CountDown
  • enable rollback for taskMaps
  • add the hint describted above.
  • add test case for it.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
  • Breaking backward compatibility

Release note

  • add return param cntPlan for find_best_task to indicate the number of plans found in it.

@danmay319 danmay319 requested a review from winoros June 8, 2020 08:32
@danmay319 danmay319 requested a review from eurekaka June 8, 2020 09:02
@codecov
Copy link

codecov bot commented Jun 8, 2020 •
edited
Loading

Codecov Report

Merging #17850 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17850   +/-   ##
===========================================
  Coverage   79.4389%   79.4389%           
===========================================
  Files           535        535           
  Lines        144190     144190           
===========================================
  Hits         114543     114543           
  Misses        20370      20370           
  Partials       9277       9277

@danmay319 danmay319 added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Jun 9, 2020
@danmay319
Copy link
Contributor Author

PTAL ~ @eurekaka @winoros

@danmay319 danmay319 changed the title planner, add return param cntPlan for find_best_task to indicate the number of plans found in it. planner, add hint eth_plan(x) to help user force a plan Jun 10, 2020
eurekaka
eurekaka reviewed Jun 30, 2020
View reviewed changes
@danmay319 danmay319 requested a review from eurekaka June 30, 2020 04:18
time-and-fate
time-and-fate reviewed Jun 30, 2020
View reviewed changes
eurekaka
eurekaka reviewed Jun 30, 2020
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot
Copy link
Contributor

@eurekaka,Thanks for you review.

@danmay319 danmay319 added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 30, 2020
@danmay319
Copy link
Contributor Author

danmay319 commented Jun 30, 2020 •
edited
Loading

PTAL @winoros , @time-and-fate

winoros
winoros approved these changes Jul 1, 2020
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@danmay319 danmay319 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 1, 2020
@ti-srebot
Copy link
Contributor

Sorry @lawyerphx, you don't have permission to trigger auto merge event on this branch. You are not a committer or co-leader or leader for the related sigs:planner(slack).

@zz-jason
Copy link
Member

zz-jason commented Jul 1, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@lawyerphx merge failed.

@danmay319
Copy link
Contributor Author

/run-unit-tests

@danmay319
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Sorry @lawyerphx, you don't have permission to trigger auto merge event on this branch. You are not a committer or co-leader or leader for the related sigs:planner(slack).

@danmay319
Copy link
Contributor Author

please merge @zz-jason

@zz-jason zz-jason merged commit dce0e45 into pingcap:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@time-and-fate time-and-fate time-and-fate left review comments

@winoros winoros winoros approved these changes

@zz-jason zz-jason Awaiting requested review from zz-jason

Assignees
No one assigned
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danmay319 @eurekaka @ti-srebot @zz-jason @winoros @time-and-fate