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

benchmark: add lightning import for TPC-C #2169

Merged
merged 7 commits into from
Jan 10, 2020
Merged

benchmark: add lightning import for TPC-C #2169

merged 7 commits into from
Jan 10, 2020

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Jan 8, 2020

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What is changed, added or deleted?

Add lightning import for TPC-C.

What is the related PR or file link(s)?

Which version does your change affect?

dev, 3.0, 3.1

@jackysp
add lightning import
da16cda 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Jan 8, 2020

I'll update 3.0 and 3.1 when it is ready to merge.

kennytm
kennytm reviewed Jan 8, 2020
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Rest LGTM

dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated
Comment on lines 229 to 232
* 在 Importer 目录下创建一个 log 文件夹
* 在 Importer 目录下执行 `scripts/start_importer.sh`，启动 Importer
* 在 Lightning 目录下创建一个 log 文件夹
* 在 Lightning 目录下执行 `scripts/start_lightning.sh`，开始导入数据
Copy link
Contributor

@kennytm kennytm Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The log folders should be created automatically by the shell scripts 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

\cc @liubo0127

Copy link
Contributor

Choose a reason for hiding this comment

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

All directorys should be created automatically. I'll check it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in pingcap/tidb-ansible#1103

Copy link
Member Author

Choose a reason for hiding this comment

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

great

jackysp and others added 2 commits January 8, 2020 13:12
@jackysp @kennytm
Update dev/benchmark/how-to-run-tpcc.md
3163e68 
Co-Authored-By: kennytm <kennytm@gmail.com>
@jackysp @kennytm
Apply suggestions from code review
3de3525 
Co-Authored-By: kennytm <kennytm@gmail.com>
jackysp
jackysp commented Jan 8, 2020
View reviewed changes
dev/benchmark/how-to-run-tpcc.md Outdated

```yaml
tikv_importer:
addr: "172.16.5.34:13323" # 这里指向 importer 的地址，需要将上面 inventory.ini 里指定的再写一次
Copy link
Member Author

Choose a reason for hiding this comment

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

Could we improve this? I mean it doesn't need to set IP:port once again. @kennytm @liubo0127

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be configured automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have tested, and it can be configured automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

企业微信截图_20200110102808
企业微信截图_20200110102653

Copy link
Member Author

Choose a reason for hiding this comment

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

port does not set automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, it's actually set automatically, but it doesn't match the custom port.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

great, thx

@lilin90 lilin90 added size/large Changes of a large size. translation/doing This PR’s assignee is translating this PR. labels Jan 8, 2020
yikeke
yikeke reviewed Jan 9, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

Some small editing advice on improving the language. Rest LGTM.

dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
dev/benchmark/how-to-run-tpcc.md Outdated Show resolved Hide resolved
@jackysp @yikeke
Apply suggestions from code review
11391c9 
Co-Authored-By: Keke Yi <40977455+yikeke@users.noreply.github.com>
yikeke
yikeke reviewed Jan 9, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

Language LGTM

@jackysp
remove fix steps
a47d1da 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
apply to v3.0 and v3.1
06c6a4a 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Jan 10, 2020

All comments addressed, and apply to v3.0 and v3.1, PTAL @yikeke

yikeke
yikeke approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

LGTM

@yikeke yikeke added the status/can-merge Indicates a PR has been approved by a committer. label Jan 10, 2020
@claassistantio
Copy link

claassistantio commented Jan 10, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@sre-bot
Copy link
Contributor

sre-bot commented Jan 10, 2020

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Jan 10, 2020

... the bot blocks CLA.

@yikeke
Copy link
Contributor

yikeke commented Jan 10, 2020 •
edited
Loading

... the bot blocks CLA.

I asked the bot's manager @you06 to sign it. Wait a moment.

@you06
Copy link
Contributor

you06 commented Jan 10, 2020

Facing a bot-login issue, we're trying to solve this.

@sre-bot sre-bot merged commit 0b052a7 into pingcap:master Jan 10, 2020
@yikeke yikeke assigned toutdesuite and unassigned yikeke Mar 4, 2020
@yikeke yikeke added priority/P1 The issue has P1 priority. and removed priority/P1 The issue has P1 priority. labels Mar 4, 2020
@jackysp jackysp deleted the tpcc_lightning branch March 18, 2020 06:16
@toutdesuite toutdesuite mentioned this pull request Mar 19, 2020
Merged
4 tasks
@toutdesuite toutdesuite added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR’s assignee is translating this PR. labels Mar 19, 2020
july2993
july2993 reviewed Mar 21, 2020
View reviewed changes
dev/benchmark/how-to-run-tpcc.md
LS1 ansible_host=172.16.5.34 deploy_dir=/data2/ls1 tidb_lightning_pprof_port=23323 data_source_dir=/home/user/csv
```

##### 修改 conf/tidb-lightning.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

not toml? @kennytm

Copy link
Contributor

Choose a reason for hiding this comment

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

YAML is correct, Ansible uses YAML as template. 😓 @july2993

Copy link
Contributor

Choose a reason for hiding this comment

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

I methinks this is the config file lightning use directly 😓

This was referenced Mar 24, 2020
Merged
Merged
@jackysp
Copy link
Member Author

jackysp commented Mar 24, 2020

The issue found in this PR is fixed by pingcap/tidb-ansible#1105, pingcap/tidb-ansible#1104 and pingcap/tidb-ansible#1103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

@july2993 july2993 july2993 left review comments

@liubo0127 liubo0127 liubo0127 left review comments

@yikeke yikeke yikeke approved these changes

@IANTHEREAL IANTHEREAL Awaiting requested review from IANTHEREAL

Assignees

@toutdesuite toutdesuite

Labels
size/large Changes of a large size. status/can-merge Indicates a PR has been approved by a committer. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@jackysp @claassistantio @sre-bot @yikeke @you06 @kennytm @july2993 @liubo0127 @toutdesuite @lilin90