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

pd-simulator: update import-data case #2741

Merged
merged 11 commits into from
Aug 11, 2020
Merged

pd-simulator: update import-data case #2741

merged 11 commits into from
Aug 11, 2020

Conversation

ZenoTan
Copy link
Contributor

@ZenoTan ZenoTan commented Aug 11, 2020

What problem does this PR solve?

Import data case is a case in pd simulator. Originally, it has 40 regions, and it imports many regions afterwards. Then import regions >> previous regions. The real scenario is, import regions << previous regions. So now it imports regions 10% of previous size.

What is changed and how it works?

Changed import data case.

Check List

Tests

  • No code

Release note

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Aug 11, 2020
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

Will it be helpful to the previous issue?

@@ -40,8 +40,8 @@ func newImportData() *Case {
})
}

storeIDs := rand.Perm(3)
for i := 0; i < 40; i++ {
for i := 0; i < 10000; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be configured?

Copy link
Contributor Author

@ZenoTan ZenoTan Aug 11, 2020

Choose a reason for hiding this comment

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

Changed to configurable.

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

Will it be helpful to the previous issue?

Which one...

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

PTAL @nolouch

@lhy1024
Copy link
Contributor

lhy1024 commented Aug 11, 2020

Will it be helpful to the previous issue?

Which one...

#1775

checkCount++
dev := 0.0
for _, p := range regionProps {
dev += (p - 10) * (p - 10) / 100
Copy link
Contributor

Choose a reason for hiding this comment

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

will it still be 10 after region num is configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means if balanced, store each get 10 % of data, no matter what the total is.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

Will it be helpful to the previous issue?

Which one...

#1775

In fact, after changing to the new scenario, because import data(3 out of 10 peers) makes it less balanced, we don't need to set a stop time, but we should observe the deviation. If schedulers work properly, balance should not be greatly affected by the import.

@lhy1024
Copy link
Contributor

lhy1024 commented Aug 11, 2020

Will it be helpful to the previous issue?

Which one...

#1775

In fact, after changing to the new scenario, because import data(3 out of 10 peers) makes it less balanced, we don't need to set a stop time, but we should observe the deviation. If schedulers work properly, balance should not be greatly affected by the import.

This behavior seems to be different compared to other simulators

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

Will it be helpful to the previous issue?

Which one...

#1775

In fact, after changing to the new scenario, because import data(3 out of 10 peers) makes it less balanced, we don't need to set a stop time, but we should observe the deviation. If schedulers work properly, balance should not be greatly affected by the import.

This behavior seems to be different compared to other simulators

Yes. It is because the new scenario is an unbalanced import. Indeed, it is trivial to add a stop condition.

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

PTAL @nolouch @lhy1024

Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

LGTM. we also need to manual test.

@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

LGTM. we also need to manual test.

It will stop at tick 1009 if region number is 10000.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 11, 2020
Copy link
Contributor

@nolouch nolouch 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 ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 11, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 11, 2020
@ZenoTan
Copy link
Contributor Author

ZenoTan commented Aug 11, 2020

/merge

@lhy1024
Copy link
Contributor

lhy1024 commented Aug 11, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 11, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 4350873 into tikv:master Aug 11, 2020
disksing pushed a commit to oh-my-tidb/pd that referenced this pull request Aug 19, 2020
* Fix mv peer log parse error

* Delete unused var

* Rename

* Update case

* Address comment

* Update import_data.go

* Add stop condition

Co-authored-by: ShuNing <nolouch@gmail.com>
ZenoTan added a commit to ZenoTan/pd that referenced this pull request Aug 24, 2020
* Fix mv peer log parse error

* Delete unused var

* Rename

* Update case

* Address comment

* Update import_data.go

* Add stop condition

Co-authored-by: ShuNing <nolouch@gmail.com>
Signed-off-by: ZenoTan <zenotan1998@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants