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

*: remove tikv-importer #5036

Merged
merged 3 commits into from Jul 8, 2019
Merged

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Jul 5, 2019

Signed-off-by: kennytm kennytm@gmail.com

What have you changed? (mandatory)

The KV importer service has been extracted into the tikv/importer repository. Only the SST importer service needs to remain here. All code related to the KV importer has been removed here.

What are the type of the changes? (mandatory)

  • Engineering (engineering change which doesn't change any feature or fix any issue)

How has this PR been tested? (mandatory)

cargo check

Does this PR affect documentation (docs) or release note? (mandatory)

  • May need to be included in the release note for the source code location.

Does this PR affect tidb-ansible update? (mandatory)

No, the binary has already been switched to be packaged from tikv/importer.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@kennytm kennytm added the component/build Component: Build, Deployment, etc. label Jul 5, 2019
@kennytm kennytm force-pushed the remove-importer-binary branch 2 times, most recently from 54ca51d to 5f439af Compare July 5, 2019 19:59
The KV importer service has been extracted into the tikv/importer
repository. Only the SST importer service needs to remain here. All code
related to the KV importer has been removed here.

Signed-off-by: kennytm <kennytm@gmail.com>
@kennytm
Copy link
Contributor Author

kennytm commented Jul 6, 2019

/run-integration-tests

@siddontang
Copy link
Contributor

If we can support a plugin which can let the user register their own gRPC server, maybe we can make code cleaner, right? @kennytm

@kennytm
Copy link
Contributor Author

kennytm commented Jul 6, 2019

@siddontang Certainly. But that may make using Importer harder, since that ImportSSTService plugin will need to be installed on every TiKV node.

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

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

LGTM

@overvenus
Copy link
Member

Could you move src/import to components/import?

@kennytm
Copy link
Contributor Author

kennytm commented Jul 8, 2019

@overvenus OK, will move in another PR.

@kennytm
Copy link
Contributor Author

kennytm commented Jul 8, 2019

/run-integration-tests

@kennytm
Copy link
Contributor Author

kennytm commented Jul 8, 2019

https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/tikv_ghpr_test/detail/tikv_ghpr_test/6452/pipeline/

thread 'raftstore-1-1::test_node_request_snapshot_reject_merge' panicked at 'called `Result::unwrap()` on an `Err` value: "SendError(..)"', src/libcore/result.rs:999:5

I guess it is spurious.

/test

@kennytm
Copy link
Contributor Author

kennytm commented Jul 8, 2019

/run-integration-common-test

@kennytm kennytm merged commit 09de060 into tikv:master Jul 8, 2019
@kennytm kennytm deleted the remove-importer-binary branch July 8, 2019 04:16
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
The KV importer service has been extracted into the tikv/importer
repository. Only the SST importer service needs to remain here. All code
related to the KV importer has been removed here.

Signed-off-by: kennytm <kennytm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/build Component: Build, Deployment, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants