cdc: avoid deadlock on error in pubsub sink #88289
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @shermanCRL)
pkg/ccl/changefeedccl/sink_pubsub.go line 362 at r1 (raw file):
} p.mu.Unlock() // openTopic may need the lock to record an error topic, err := p.openTopic(name)
openTopic seems to only be called from this function and record error is only called from openTopic; why not just rename openTopic to openTopicLocked (and possible recordError to recordErrorLocked)?
HonoreDB
force-pushed
the
fix_deadlock_on_autocreate_error
branch
from
c46a9d0 to
4a3f357
Compare
cockroachdb#88130 introduced a deadlock when an attempt to create a topic fails -- the goroutine tries to acquire a lock in order to record the error, but it already has it in order to write to the map. This PR releases the lock while creating the topic, which should also help with performance a bit on startup. Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.
HonoreDB
force-pushed
the
fix_deadlock_on_autocreate_error
branch
from
4a3f357 to
00a4abe
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/sink_pubsub.go line 362 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
openTopic seems to only be called from this function and record error is only called from openTopic; why not just rename openTopic to openTopicLocked (and possible recordError to recordErrorLocked)?
Done.
|
bors r+ |
|
Build succeeded: |
#88130 introduced a deadlock when an attempt to create a
topic fails -- the goroutine tries to acquire a lock in order to record the error, but it already has it in order to write to the map. This PR releases the lock while creating the topic, which should also help with performance a bit on startup.
Fixes #85374
Release note (bug fix): Fixed a bug preventing pubsub changefeeds from retrying.