Skip to content

*: support new protoc w/ dtpb#2096

Merged
ti-srebot merged 4 commits intopingcap:masterfrom
SchrodingerZhu:fix-dtpb
Jun 9, 2021
Merged

*: support new protoc w/ dtpb#2096
ti-srebot merged 4 commits intopingcap:masterfrom
SchrodingerZhu:fix-dtpb

Conversation

@SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Jun 6, 2021

Signed-off-by: SchrodingerZhu i@zhuyi.fan

What problem does this PR solve?

Issue Number: partially #2064

If we update dtpb, the base class of RegionState will be marked as final.

What is changed and how it works?

What's Changed:

change from inheritance to proxy.

Check List

Tests

  • Unit test
  • Integration test

Release note

No release note

Signed-off-by: SchrodingerZhu <i@zhuyi.fan>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@SchrodingerZhu SchrodingerZhu changed the title : support new protoc w/ dtpb *: support new protoc w/ dtpb Jun 7, 2021
Signed-off-by: SchrodingerZhu <i@zhuyi.fan>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@JaySon-Huang JaySon-Huang left a comment

Choose a reason for hiding this comment

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

https://github.com/pingcap/tics/blob/a48fac743bf8a9427b5d572b68662ab475a04ea5/dbms/src/Storages/Transaction/RegionState.cpp#L9-L17

Instead of (Base &)* this = (Base &&) from;, I think using base = std::move(from.base); is more explicit?

@SchrodingerZhu
Copy link
Contributor Author

@JaySon-Huang let me clean up the thing here and you can simply merge w/ your existing one?

Signed-off-by: SchrodingerZhu <i@zhuyi.fan>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor

coalescing into #2110

This PR changing the RegionState from "is-a" to "has-a" with raft_serverpb::RegionLocalState.
#2110 change the pb.cc/pb.h files generated by CMake.
Logically, the two changes are independent, so I think it is better to keep this PR as a standalone commit to the master branch.

Copy link
Contributor

@JaySon-Huang JaySon-Huang 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 added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 9, 2021
@SchrodingerZhu
Copy link
Contributor Author

SchrodingerZhu commented Jun 9, 2021

got it.

@SchrodingerZhu
Copy link
Contributor Author

/merge

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

/run-all-tests

@ti-srebot
Copy link
Collaborator

@SchrodingerZhu merge failed.

@SchrodingerZhu
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot ti-srebot merged commit 7a24acf into pingcap:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants