streamingest: wait for all goroutines on streamIngestProcessor shutdown #72650
streamingest: wait for all goroutines on streamIngestProcessor shutdown #72650craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
The checked chan is never nil. Release note: None
This processor was failing to stop and wait for some of the goroutines it spawned, which is not nice (in particular, it's likely that those goroutines were using a tracing span after it was finished). Release note: None
|
This change is |
|
I pulled this out of the bigger PR cause I had gotten it wrong. |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @andreimatei)
pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go, line 313 at r3 (raw file):
sip.pollingWaitGroup.Wait() // Wait for the merge goroutine. sip.cancelMergeAndWait()
In an error case it's possible that cancelMergeAndWait is never set (i.e. we short-circuit Start method before merge is called), so it might be worth either checking here for nil or set this function to a non-nil noop in the constructor.
This processor was calling InternalClose() while goroutines were still running. InternalClose() finishes the processor's tracing span, which is used by those goroutines. We don't like span use-after-Finish. Release note: None
3f02ba5 to
42db1dd
Compare
andreimatei
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go, line 313 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
In an error case it's possible that
cancelMergeAndWaitis never set (i.e. we short-circuitStartmethod beforemergeis called), so it might be worth either checking here fornilor set this function to a non-nil noop in the constructor.
done
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
This processor was failing to stop and wait for some of the goroutines it
spawned, which is not nice (in particular, it's likely that those
goroutines were using a tracing span after it was finished).
Release note: None