Skip to content

Better integration with tipb, kvproto#2110

Merged
JaySon-Huang merged 4 commits intopingcap:masterfrom
JaySon-Huang:refine_pb_integration
Jun 11, 2021
Merged

Better integration with tipb, kvproto#2110
JaySon-Huang merged 4 commits intopingcap:masterfrom
JaySon-Huang:refine_pb_integration

Conversation

@JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Jun 7, 2021 •
edited
Loading

Signed-off-by: JaySon-Huang jayson.hjs@gmail.com

What problem does this PR solve?

Issue Number: a part of #2019

Problem Summary:

  • We need to run contrib/tipb/generate-cpp.sh, contrib/kvproto/scripts/generate_cpp.sh manually if tipb,kvproto updated
  • We commit the ".pb.cc" and ".pb.h" files, which is not the best practice. It brings trouble for upgrading our protobuf/grpc version. Should generate related files by the cmake function protobuf_generate_cpp

What is changed and how it works?

Automatically re-generate .pb.h and .pb.cc if .proto file changed, don't need to run contrib/tipb/generate-cpp.sh, contrib/kvproto/scripts/generate_cpp.sh manually.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch:

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    • Build ok

Side effects

Release note

  • No release note

@JaySon-Huang JaySon-Huang self-assigned this Jun 7, 2021
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 7, 2021

Waiting for pingcap/tipb#230 and pingcap/kvproto#775

@JaySon-Huang JaySon-Huang changed the title Better integration with protobuf Better integration with tipb, kvproto Jun 7, 2021
@JaySon-Huang JaySon-Huang mentioned this pull request Jun 7, 2021
Open
18 tasks
@JaySon-Huang JaySon-Huang force-pushed the refine_pb_integration branch from 49e3c40 to dee2037 Compare June 7, 2021 03:03
@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 7, 2021

@JaySon-Huang Is it also doable to consider the files mentioned in #2096?

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 7, 2021 •
edited
Loading

@JaySon-Huang Is it also doable to consider the files mentioned in #2096?

README is updated.
I can merge your changes in dbms/src/Storages/Transaction/RegionState.{h,cpp} if you agree. It is also fine to keep your PR as a standalone one.

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 7, 2021

/run-all-tests

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 8, 2021

perhaps update .gitignore?

@SchrodingerZhu SchrodingerZhu mentioned this pull request Jun 8, 2021
Closed
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 8, 2021

perhaps update .gitignore?

I am not sure which files you want to add to .gitignore? Those files generated by protobuf_generate_cpp are stored at the build directory but not the source directory.

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 8, 2021

@JaySon-Huang I see. then it is fine. great!

@SchrodingerZhu SchrodingerZhu mentioned this pull request Jun 8, 2021
Merged
@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 9, 2021

LGTM

@ti-srebot

This comment has been minimized.

Sign in to view
@JaySon-Huang JaySon-Huang force-pushed the refine_pb_integration branch from ff614f9 to 05191d0 Compare June 9, 2021 07:10
@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 9, 2021

/rebuild

1 similar comment
@solotzg
Copy link
Contributor

solotzg commented Jun 9, 2021

/rebuild

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Jun 9, 2021

/run-all-tests

@lidezhu lidezhu self-requested a review June 10, 2021 02:47
lidezhu
lidezhu reviewed Jun 10, 2021
View reviewed changes
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 11, 2021
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jun 11, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 11, 2021
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021

/merge

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

ti-srebot commented Jun 11, 2021

/run-all-tests

@ti-srebot
Copy link
Collaborator

ti-srebot commented Jun 11, 2021

@JaySon-Huang merge failed.

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021

/merge

@ti-srebot
Copy link
Collaborator

ti-srebot commented Jun 11, 2021

Your auto merge job has been accepted, waiting for:

  • 2139

@ti-srebot
Copy link
Collaborator

ti-srebot commented Jun 11, 2021

@JaySon-Huang merge failed.

@JaySon-Huang
Better integration with protobuf
81659de 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang
Clean release-darwin
8753588 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang
Add -Wno-unused-paramter for dtpb
827b4f9 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang
Update kvproto & tipb
c081bee 
Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>
@JaySon-Huang JaySon-Huang force-pushed the refine_pb_integration branch from 2f554e2 to c081bee Compare June 11, 2021 10:47
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021

/merge

@ti-srebot
Copy link
Collaborator

ti-srebot commented Jun 11, 2021

/run-all-tests

@ti-srebot
Copy link
Collaborator

ti-srebot commented Jun 11, 2021

@JaySon-Huang merge failed.

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021 

[2021-06-11T11:38:30.328Z] ERROR: Queue task was cancelled

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021

/run-all-tests

1 similar comment
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Jun 11, 2021

/run-all-tests

@JaySon-Huang JaySon-Huang merged commit 8053cac into pingcap:master Jun 11, 2021
@JaySon-Huang JaySon-Huang deleted the refine_pb_integration branch June 11, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidezhu lidezhu lidezhu approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

+1 more reviewer

@hanfei1991 hanfei1991 hanfei1991 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JaySon-Huang JaySon-Huang

Labels

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.

6 participants

@JaySon-Huang @SchrodingerZhu @ti-srebot @solotzg @hanfei1991 @lidezhu