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

*: update dependencies #9715

Merged
merged 6 commits into from
Mar 1, 2021
Merged

*: update dependencies #9715

merged 6 commits into from
Mar 1, 2021

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Feb 25, 2021

What problem does this PR solve?

Issue Number: close #9192

Problem Summary:

update grpcio to 0.8, also update kvproto, tipb, rocksdb, raft to its
latest main branch to include bugfixes and make them compile on Mac m1.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Release note

  • Update several dependencies to make them compile on Mac m1

@BusyJay
*: update dependencies
c61f649 
update grpcio to 0.8, also update kvproto, tipb, rocksdb, raft to its
latest main branch to include bugfixes and make them compile on Mac m1.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2021
@BusyJay
update more deps
6f8bbba 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2021
@BusyJay
Copy link
Member Author

BusyJay commented Feb 25, 2021

Verified it compiled and tested on Mac m1.

$ make clean
$ env EXTRA_CARGO_ARGS=--no-run make test
    Building [=====================>   ] 680/765
    Finished test [unoptimized + debuginfo] target(s) in 6m 16s
$ make clean
$ make release
    Building [=======================> ] 605/609: cmd, tikv
    Finished release [optimized + debuginfo] target(s) in 21m 21s

Compilations are run with power adaptor plugged in. MacOS will slow things down when compilation consumes too much energy (take more than 6% battery to do release build). As a comparison, I also compile TiKV on a 8 core virtual machine:

$ make clean
$ env EXTRA_CARGO_ARGS=--no-run make test
    Building [================>        ] 580/820
    Finished test [unoptimized + debuginfo] target(s) in 13m 58s
$ make clean
$ make release
    Building [=====================>   ] 549/613
    Finished release [optimized] target(s) in 21m 01s

The comparison is obvious unfair as we enable some optimizations (more compilation work) on Linux only and m1's CPU frequency is 1.6 times of linux vm.

CI failure is due to outdated gcc on CentOS 7. Latest grpc requires gcc 4.9+, see also grpc/grpc#24932.

@sticnarf sticnarf mentioned this pull request Feb 26, 2021
Closed
4 tasks
@BusyJay
Copy link
Member Author

BusyJay commented Feb 27, 2021

/run-all-tests

4 similar comments
@BusyJay
Copy link
Member Author

BusyJay commented Feb 27, 2021

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@BusyJay
Copy link
Member Author

BusyJay commented Feb 27, 2021

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@BusyJay BusyJay marked this pull request as ready for review February 28, 2021 04:28
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2021
@BusyJay
Copy link
Member Author

BusyJay commented Feb 28, 2021

/bench

@BusyJay
Copy link
Member Author

BusyJay commented Feb 28, 2021

/release

@sre-bot
Copy link
Contributor

sre-bot commented Feb 28, 2021

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/57ba323b207a41906ae7967792bcff8f2cf1f84c/centos7/tikv-server.tar.gz

sticnarf
sticnarf reviewed Mar 1, 2021
View reviewed changes
Cargo.lock Outdated
@@ -505,7 +511,7 @@ dependencies = [
"panic_hook",
"pd_client",
"prometheus",
"prost",
"prost 0.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also update the direct dependency of prost in TiKV from 0.6 to 0.7?

@BusyJay
update prost
c671ea4 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 1, 2021
@BusyJay
Copy link
Member Author

BusyJay commented Mar 1, 2021

Turns out there is no prost check in CI and it's broken (in master as well).

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021
@sticnarf
Copy link
Contributor

sticnarf commented Mar 1, 2021 •
edited
Loading

Turns out there is no prost check in CI and it's broken (in master as well).

I also found it before. The issue I met is the wrong feature resolved from raft-rs. Without resolver v2, the raft-proto crate, which is an inner crate in raft-rs, has both protobuf-codec and prost-codec enabled.

@BusyJay
Copy link
Member Author

BusyJay commented Mar 1, 2021

Without resolver v2, ...

Isn't it enabled in the Cargo.toml of TiKV project? Besides default features are disabled in all references.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hicqu
  • sticnarf

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 1, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021
@hicqu
Copy link
Contributor

hicqu commented Mar 1, 2021

/merge

@ti-chi-bot
Copy link
Member

@hicqu: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 411c607

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 1, 2021
@hicqu
Copy link
Contributor

hicqu commented Mar 1, 2021

/run-all-tests

@hicqu hicqu merged commit 47e03ae into tikv:master Mar 1, 2021
@BusyJay
Copy link
Member Author

BusyJay commented Mar 1, 2021

Benchmark shows there are 1% difference in throughput, which is within error range.

@BusyJay BusyJay deleted the update-grpcio branch March 1, 2021 07:54
@BusyJay
Copy link
Member Author

BusyJay commented Mar 3, 2021

A more detail bench table in case anyone is interested.

- point select oltp read only tpmc ycsb update ycsb read ycsb scan
master 70835.78 31611.99 25411.4 13m31 3m52 35m35
this branch 70161.35 31381.10 25461.6 13m28 3m54 36m41
diff -0.95% 0.73% 0.2% -0.37% 0.86% 3.09%

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

@hicqu hicqu hicqu approved these changes

@sticnarf sticnarf sticnarf approved these changes

Assignees
No one assigned
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support build on Apple Silicon/M1
6 participants
@BusyJay @zhouqiang-cl @sre-bot @sticnarf @ti-chi-bot @hicqu