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

debugpb: support fail point #200

Merged
merged 15 commits into from
Oct 16, 2017
Merged

debugpb: support fail point #200

merged 15 commits into from
Oct 16, 2017

Conversation

overvenus
Copy link
Member

Fail point API for injecting failures!

@lishuai87
Copy link
Contributor

LGTM

@@ -99,3 +103,22 @@ message ScanMvccResponse {
bytes key = 1;
kvrpcpb.MvccInfo info = 2;
}

message Failure {
enum Type {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should figure out a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add two methods instead, one for setting and one for removal.

message Failure {
enum Type {
INVALID = 0;
INJECT = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we remove INVALID and set INJECT as default?

}

message FailPointRequest {
repeated Failure failures = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be one request for one failure?

Let's consider failures [A, B, C], we may set A and B successfully but C failed, so what should we do? Revert A and B then return error to client? But it is possible that A or B is already be triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just one should be OK.

@siddontang
Copy link
Member

PTAL @BusyJay

@@ -38,6 +38,10 @@ service Debug {
// Note: DO NOT CALL IT IN PRODUCTION, it's really expensive.
// Server uses keys directly w/o any encoding.
rpc ScanMvcc(ScanMvccRequest) returns (stream ScanMvccResponse) {}

// Inject fail points. Currently, it only used in tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/it/it's/

}

message FailPointRequest {
repeated Failure failures = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one should be OK.

@@ -99,3 +103,22 @@ message ScanMvccResponse {
bytes key = 1;
kvrpcpb.MvccInfo info = 2;
}

message Failure {
enum Type {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add two methods instead, one for setting and one for removal.

@overvenus
Copy link
Member Author

PTAL

@BusyJay
Copy link
Contributor

BusyJay commented Sep 30, 2017

LGTM

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2017

CLA assistant check
All committers have signed the CLA.

@siddontang
Copy link
Member

LGTM
PTAL @BusyJay

@zhangjinpeng87
Copy link
Contributor

LGTM

@overvenus overvenus merged commit 146f027 into master Oct 16, 2017
@overvenus overvenus deleted the ov/fail-point branch October 16, 2017 03:10
@overvenus overvenus restored the ov/fail-point branch October 16, 2017 03:10
@overvenus overvenus deleted the ov/fail-point branch October 16, 2017 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants