Watch events during linearizability test and compare history #15044
Conversation
serathius
force-pushed
the
linearizability-watch
branch
from
4c21602 to
eedbb50
Compare
Codecov Report
@@ Coverage Diff @@
## main #15044 +/- ##
==========================================
- Coverage 74.95% 74.60% -0.35%
==========================================
Files 415 415
Lines 34288 34288
==========================================
- Hits 25700 25580 -120
- Misses 6970 7069 +99
- Partials 1618 1639 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
serathius
changed the title
|
PTAL @ahrtr |
| trafficCtx, trafficCancel := context.WithCancel(ctx) | ||
| go func() { | ||
| defer cancel() | ||
| err := triggerFailpoints(ctx, t, clus, failpoint) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } | ||
| defer trafficCancel() | ||
| triggerFailpoints(ctx, t, clus, failpoint) | ||
| // Wait second to collect traffic after triggering last failpoint. | ||
| time.Sleep(time.Second) | ||
| }() | ||
| operations := simulateTraffic(ctx, t, clus, traffic) | ||
| watchCtx, watchCancel := context.WithCancel(ctx) | ||
| var operations []porcupine.Operation | ||
| go func() { | ||
| defer watchCancel() | ||
| operations = simulateTraffic(trafficCtx, t, clus, traffic) | ||
| // Wait second to collect watch events after all traffic was sent. | ||
| time.Sleep(time.Second) | ||
| }() | ||
| events := watchClusterChanges(watchCtx, t, clus) |
There was a problem hiding this comment.
You'd better add comment to explain the relationship between three kinds of goroutines and how they are related by the cancel functions,
- The triggering failpoint goroutine
- The simulating traffic goroutine
- The watching event goroutines.
There was a problem hiding this comment.
Comments should explain "why?" and not "what?". Isn't it clear in:
go func() {
defer trafficCancel()
triggerFailpoints(ctx, t, clus, failpoint)
// Wait second to collect traffic after triggering last failpoint.
time.Sleep(time.Second)
}()
The failpoints gorouting cancels traffic?
There was a problem hiding this comment.
Isn't it clear
It's clear to you because you are the author. It doesn't mean it's clear to other contributors. If I understood it correctly, the The triggering failpoint goroutine stops the simulating traffic goroutine by executing defer trafficCancel(), and the simulating traffic goroutine stops the watching event goroutines by defer watchCancel().
Note that it isn't Intuitive at first look. I was even thinking it might be a copy-paste typo, because I was confused why you use ctx, but defer cancel the trafficCancel.
It isn't the first time I raise similar comment. Please always keep in mind to add as much self-contained comment as possible.
There was a problem hiding this comment.
It's clear to you because you are the author. It doesn't mean it's clear to other contributors.
This is a very vague generic statement. Code is clean or it's not. If concept of using context to propagate stop signal is not familiar or standard then let's rewrite it. It's not about throwing as many comments as possible as it doesn't solve the problem with code.
Moved the code to separate function added comment explaining idea to use context to propagate cancellation.
serathius
force-pushed
the
linearizability-watch
branch
from
eedbb50 to
c4ea991
Compare
serathius
force-pushed
the
linearizability-watch
branch
2 times, most recently
from
7640095 to
221d2bb
Compare
| @@ -196,6 +204,18 @@ type trafficConfig struct { | |||
| traffic Traffic | |||
| } | |||
|
|
|||
| func validateEventsMatch(t *testing.T, ops [][]watchEvent) { | |||
| for i := 1; i < len(ops); i++ { | |||
There was a problem hiding this comment.
Please add a comment describing when we expect different lengths... (one node off/partitioned during the test).
serathius
force-pushed
the
linearizability-watch
branch
from
221d2bb to
57dfc87
Compare
Signed-off-by: Marek Siarkowicz <[email protected]>
Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
linearizability-watch
branch
from
57dfc87 to
8a9f848
Compare
|
|
||
| func collectClusterWatchEvents(ctx context.Context, t *testing.T, clus *e2e.EtcdProcessCluster) [][]watchEvent { | ||
| mux := sync.Mutex{} | ||
| var wg sync.WaitGroup |
cc @ahrtr @spzala @ptabor