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

Feature Request: "Support 'set' command to enable failpoint" in vtgate #247

Closed
gerayking opened this issue Aug 30, 2023 · 21 comments · Fixed by #254
Closed

Feature Request: "Support 'set' command to enable failpoint" in vtgate #247

gerayking opened this issue Aug 30, 2023 · 21 comments · Fixed by #254
Assignees
Labels
CI/CD enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed Need Discussion Testing

Comments

@gerayking
Copy link
Contributor

Feature Description

Currently, injecting failpoints is only supported via environment variables or direct code injection. However, in most cases, we need to dynamically set failpoints to enable or disable them. Therefore, the best solution is to add a 'set' command that allows enabling or disabling fail injection on the fly.

Use Case(s)

No response

@earayu
Copy link
Collaborator

earayu commented Aug 30, 2023

I love the idea.

I think it doesn't have to be a 'set command', it can also be an internal function, even a HTTP or gPRC API (HTTP/gRPC may not be as convienent as internal function, BTW).

@earayu earayu added help wanted Extra attention is needed good first issue Good for newcomers labels Aug 30, 2023
@gerayking gerayking self-assigned this Aug 30, 2023
@gerayking
Copy link
Contributor Author

I am looking to incorporate some 'set' commands in vtgate,
for example:

set @add_failpoint = 'key=value';
set @remove_failpoint = 'key';
select @show_failpoint;

@earayu
Copy link
Collaborator

earayu commented Aug 30, 2023

I am looking to incorporate some 'set' commands in vtgate, for example:

set @add_failpoint = 'key=value';
set @remove_failpoint = 'key';
select @show_failpoint;

I think put is better than add, because it's semantic should be updating the failpoint map, rather than adding an item to the failpoint map.

@earayu
Copy link
Collaborator

earayu commented Aug 30, 2023

How about the vttablet?
When set command is called, the VTGate will process it. failpoints should be passed to vttablet in some way.

@gerayking
Copy link
Contributor Author

I am looking to incorporate some 'set' commands in vtgate, for example:

set @add_failpoint = 'key=value';
set @remove_failpoint = 'key';
select @show_failpoint;

I think put is better than add, because it's senmantic should be updating the failpoint map, rather than adding an item to the failpoint map.

yeah, I also agree with you.

@gerayking
Copy link
Contributor Author

How about the vttablet? When set command is called, the VTGate will process it. failpoints should be passed to vttablet in some way.

Consider adding the @put_failpoint(key, val, vttablet type[primary or follower or what target special vttablet parameters]) command?

@earayu
Copy link
Collaborator

earayu commented Aug 30, 2023

Consider adding the @put_failpoint(key, val, vttablet type[primary or follower or what target special vttablet parameters]) command?

The scope of failpoint should be global.
If key1=value1 is injected, it should take effect on both VTGate and VTTablet. All we need to do is find a way to pass key1=value1 to VTTablet.

One possible approach is: when set @put_failpoint = 'key1=value1’; is called, VTGate will notify all VTTablets.
Another possible approach is: when set @put_failpoint = 'key1=value1’; is called, VTGate will not notify VTTablet, but each time VTGate calls VTTablet, it will pass all failpoints together via gRPC. (Maybe through querypb.ExecuteOptions)

@gerayking
Copy link
Contributor Author

I think the former is better. I can do this by reusing reloadExec or creating new grpc interface?

@earayu
Copy link
Collaborator

earayu commented Aug 30, 2023

sure

I think the former is better. I can do this by reusing reloadExec or creating new grpc interface?

@gerayking
Copy link
Contributor Author

If we need to inject a failPoint specifically into the primary MySQL instance and not into other MySQL instances, then broadcasting might not be the best approach?

@earayu
Copy link
Collaborator

earayu commented Sep 4, 2023

If we need to inject a failPoint specifically into the primary MySQL instance and not into other MySQL instances, then broadcasting might not be the best approach?

I think the default behavior can be "broadcasting." If we need to inject to a specific vttablet, we can add some hints.
see this issue: #225

@gerayking gerayking changed the title Feature Request: "Support 'set' command to enable failpoint". Feature Request: "Support 'set' command to enable failpoint" in vtgate Sep 5, 2023
@wesql wesql deleted a comment from gerayking Sep 6, 2023
@gerayking
Copy link
Contributor Author

I am looking to incorporate some 'set' commands in vtgate, for example:

set @add_failpoint = 'key=value';
set @remove_failpoint = 'key';
select @show_failpoint;

I think put is better than add, because it's senmantic should be updating the failpoint map, rather than adding an item to the failpoint map.

yeah, I also agree with you.

How about replacing select @show_failpoint with show failpoint?

@earayu
Copy link
Collaborator

earayu commented Sep 6, 2023

I think show failpoints is fine.

@gerayking
Copy link
Contributor Author

failpoint.List() returns only those failpoints that are enabled,I think still need to maintain a map?

@earayu
Copy link
Collaborator

earayu commented Sep 6, 2023

What results do you expect from the "show failpoints" command? All defined failpoints or all enabled failpoints? @gerayking

@gerayking
Copy link
Contributor Author

What results do you expect from the "show failpoints" command? All defined failpoints or all enabled failpoints? @gerayking

I think "All defined failpoints" maybe better. This allows us to use the show failpoints command to view all available failpoints and then utilize the set command to modify as needed."

@earayu
Copy link
Collaborator

earayu commented Sep 6, 2023

Hmm, Interesting idea.
What I was considering earlier was to display all the enabled failpoints.

But how would you collect all defined failpoints?

@gerayking
Copy link
Contributor Author

I don't have a elegant solution yet, but I'm considering two approaches:

  1. Maintain a static map[string]string to track the failpoints that need dynamic adjustment.
  2. Add a registration step at the end of failpoint.inject.
    I like first more than second.

@earayu
Copy link
Collaborator

earayu commented Sep 6, 2023

I don't have a elegant solution yet, but I'm considering two approaches:

  1. Maintain a static map[string]string to track the failpoints that need dynamic adjustment.
  2. Add a registration step at the end of failpoint.inject.
    I like first more than second.

Sure you can have a "static map[string]string to track the failpoints".
But my question is: If I were to define a failpoint by invoking the failpoint.Inject("key", nil) function, how would it be added to the map you mentioned?

@earayu
Copy link
Collaborator

earayu commented Sep 6, 2023

Let me try to understand what you mean, and please correct me if I'm wrong @gerayking :

  1. I need to maintain all "failpoint keys" in your global constant map.
  2. When I need to invoke "failpoint.Inject(...)", I should not use strings but use the "failpoint keys" defined in the first step.

@gerayking
Copy link
Contributor Author

Let me try to understand what you mean, and please correct me if I'm wrong @gerayking :

  1. I need to maintain all "failpoint keys" in your global constant map.
  2. When I need to invoke "failpoint.Inject(...)", I should not use strings but use the "failpoint keys" defined in the first step.
  1. I believe it might be unnecessary to maintain all 'failpoint keys'. Instead, we should only keep track of the ones we need.

  2. you can invoke failpoint.Inject(...) from anywhere, but using the show failpointscommand will only display keys maintained in the global constant map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed Need Discussion Testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants