changefeedccl: Add pushback duration metric to blocking buffer.#68256
changefeedccl: Add pushback duration metric to blocking buffer.#68256craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
stevendanna
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)
a discussion (no related file):
Seems like a reasonable metric to add to me. Looks like there are some CI issues but otherwise seems good.
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 30 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/mon" "github.com/cockroachdb/cockroach/pkg/util/randutil"
You likely need to make bazel-generate after these import changes.
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 93 at r1 (raw file):
} // Keep consuming events until buffer enters pushback.
thoughts: It looks to me like this test depends on the 10 goroutines started above filling the buffer faster than our one goroutine can cosume the events (seems very likely given the small size of the buffer and the size of the messages). If we broke the accounting though, I think this would fail by timing out. That's probably OK given how many tests we have that operate that way but perhaps a comment to remind us would be good.
I can't think of a way to guarantee the buffer enters pushback without some testing hooks inside the blocking buffer. But, we might be able to reduce the (already small) chance of this flaking out by synchronously pre-filling the buffer before starting the async producers (or waiting to start the consumer until some number of messages have been pushed).
miretskiy
left a comment
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, stevendanna (Steven Danna) wrote…
Seems like a reasonable metric to add to me. Looks like there are some CI issues but otherwise seems good.
ack.
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 30 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
You likely need to
make bazel-generateafter these import changes.
One day I'll remember to do that. But not today.
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 93 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
thoughts: It looks to me like this test depends on the 10 goroutines started above filling the buffer faster than our one goroutine can cosume the events (seems very likely given the small size of the buffer and the size of the messages). If we broke the accounting though, I think this would fail by timing out. That's probably OK given how many tests we have that operate that way but perhaps a comment to remind us would be good.
I can't think of a way to guarantee the buffer enters pushback without some testing hooks inside the blocking buffer. But, we might be able to reduce the (already small) chance of this flaking out by synchronously pre-filling the buffer before starting the async producers (or waiting to start the consumer until some number of messages have been pushed).
I think you made a good comment, calling out my "laziness". Turns out there is a perfectly nice way of accomplishing good what this test is trying to accomplish without testing hooks.
miretskiy
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 93 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
I think you made a good comment, calling out my "laziness". Turns out there is a perfectly nice way of accomplishing good what this test is trying to accomplish without testing hooks.
Done.
miretskiy
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 30 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
One day I'll remember to do that. But not today.
Done.
stevendanna
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 76 at r2 (raw file):
// Arrange for mem buffer to notify us when it waits for resources. var waitMu syncutil.Mutex waited := sync.NewCond(&waitMu)
up to you: we could use a chan struct{} here instead. Not sure which people generally find easier to read.
miretskiy
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/kvevent/blocking_buffer_test.go, line 76 at r2 (raw file):
Previously, stevendanna (Steven Danna) wrote…
up to you: we could use a
chan struct{}here instead. Not sure which people generally find easier to read.
I was worried that I would have multiple write attempts; or read attempts; or close attempts...
I think condition here is good.
|
tftr |
|
bors r- |
|
Canceled. |
Add a blocking buffer metric which keeps track of the amount of time waited for resource aquisition. Release Notes: None
|
@stevendanna @ajwerner I made some changes suggested by Andrew re not keeping track of additional state in the blocking buffer and just making few minor changes in quota pool so that we can leverage existing functionality. |
|
bors r+ |
|
Build succeeded: |
Add a blocking buffer metric which keeps track of the amount
of time waited for resource aquisition.
Release Notes: None