-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
consolidation: Fix flaky test #8417
Conversation
Signed-off-by: Vicent Marti <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>
@@ -330,14 +329,14 @@ func TestConsolidatorMemoryLimits(t *testing.T) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does streamItemDelay
need to be 10ms above or could that be much lower at 1ms to also have a faster text execution? And the sleep window below could be kept the same, or even lower with more of a time buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff --git i/go/vt/vttablet/tabletserver/stream_consolidator_test.go w/go/vt/vttablet/tabletserver/stream_consolidator_test.go
index 97c68aeac3..f3516999a1 100644
--- i/go/vt/vttablet/tabletserver/stream_consolidator_test.go
+++ w/go/vt/vttablet/tabletserver/stream_consolidator_test.go
@@ -323,13 +323,13 @@ func TestConsolidatorMemoryLimits(t *testing.T) {
t.Run("two-phase consolidation (time)", func(t *testing.T) {
ct := consolidationTest{
cc: NewStreamConsolidator(128*1024, 2*1024, nocleanup),
- streamItemDelay: 10 * time.Millisecond,
+ streamItemDelay: 1 * time.Millisecond,
streamItemCount: 10,
}
ct.run(10, func(worker int) (string, StreamCallback) {
if worker > 4 {
- time.Sleep(150 * time.Millisecond)
+ time.Sleep(50 * time.Millisecond)
}
return "select 1", func(_ *sqltypes.Result) error {
return nil
Signed-off-by: Vicent Marti <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paired with @vmg on this and the changes make sense and address the CI issue here at hand.
Description
This test is flaky and I'm debugging it on CI
Related Issue(s)
Checklist
Deployment Notes