remove gofail-go failpoint #30
Conversation
My understanding is the main motivation to introduce gofail-go failpoint is to make sure it can't be updated by users once it's triggered. Instead, users have to delete/disable it firstly, and then set another term for the gofail-go failpoint. Two points: 1. The gofail-go's Release is called in a separate goroutine, it might cause goroutine leak. 2. It complicates the overall design & implementation without obvious benefit. Signed-off-by: Benjamin Wang <[email protected]>
ahrtr
force-pushed
the
refactor_gofail_go_20221117
branch
from
a8fdec8
to
82c6f34
Compare
|
Thanks for looking into this. It was also my suspiction that gofail-go was over complicating the code without visible benefits, however I didn't want to make assumptions as I don't know the history behind it. @ahrtr do you know how gofail was originally used for functional testing? Would be good to avoid reinventing the wheel. |
| close(donec) | ||
| } | ||
| } | ||
|
|
||
| fp.mu.RUnlock() |
There was a problem hiding this comment.
Why do we lock mutex for failpoint execution time (between Acquire and Release)? I'm just curious as I don't see any case that this would matter.
There was a problem hiding this comment.
Not sure about intention of gofail-go, however I agree with direction to remove features that we don't need/use.
Please remember to remove mention of gofail-go in other places. For example documentation https://github.com/etcd-io/gofail/blob/master/doc/design.md#example-5-with-gofail-go-label
Separate PR should be ok.
It's similar to how we use in the linearizability test, we just need to get the failpoints terms/commands defined in failpoint-commands. But the functional test has a better implementation than the what we did in the linearizablity test. It (the functional test) injects all commands in each failpoint. For example, assuming there are 100 failpoints, and we defined 2 failpoint terms/commands (e.g. panic, sleep), then there are 200 cases in total.
Yes, there was some knowledge gap, and the old maintainers are not active anymore. So We have to depend on ourselves to figure out everything that is blocking us. Basically I don't see anything critical/important that's blocking me for now.
Yes, let me update it in a separate PR. |
|
We need to think about how to unify our existing test frameworks, including E2E/linearizability, functional test... |
Interesting, I want to go in other direction. Instead of iterating through a list of failpoints and triggering them one by one, I want to do combinations. I think this is the only way to consistently reproduce etcd-io/etcd#14682. For example delay apply transaction with sleep and panic after saving on non-apply. I would really appreciate your expertise in this area. Would be great if we could make linearizability test injection better. |
Sure. Let me think about how to enhance the linearizability test. |
My understanding is the main motivation to introduce gofail-go failpoint is to make sure it can't be updated by users once it's triggered. Instead, users have to delete/disable it firstly, and then set another term for the gofail-go failpoint.
Two points:
Signed-off-by: Benjamin Wang [email protected]