bulkio: use correct context in processor initialization #60958
Conversation
|
This change is |
| @@ -176,14 +172,14 @@ func (sip *streamIngestionProcessor) Start(ctx context.Context) { | |||
| startTime := timeutil.Unix(0 /* sec */, sip.spec.StartTime.WallTime) | |||
| eventChs := make(map[streamingccl.PartitionAddress]chan streamingccl.Event) | |||
| for _, partitionAddress := range sip.spec.PartitionAddresses { | |||
| eventCh, err := sip.client.ConsumePartition(sip.Ctx, partitionAddress, startTime) | |||
| eventCh, err := sip.client.ConsumePartition(ctx, partitionAddress, startTime) | |||
There was a problem hiding this comment.
@yuzefovich Just wanted to check if there was a convention to use ctx here rather than sip.Ctx?
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)
pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go, line 175 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
@yuzefovich Just wanted to check if there was a convention to use
ctxhere rather thansip.Ctx?
I think we tend to use ctx in most places (the only exceptions are sampler and sample_aggregator, I think), so this change LGTM.
This commit ensures that producer goroutines created during the initialization of bulk processors use the same context as bp.Ctx. Release note: None
|
Ack, this has been hitting a lot of flakes. Rebased in case any tests have been updated on master/flakes addressed recently. |
|
TFTRs, merging with high priority to address the flakes |
|
Build failed: |
|
Hit a couple of flakes |
|
Build failed: |
Skipping on master while flake is resolved. Release note: None
|
@ajstorm this flaked on #60773 in https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests/2709929?. Unsure what's happening, but I added a commit that skips the entire test. We've been seeing some imports hand due to KV changes, which may be related to these restores hanging. Could I get a stamp for the second commit? |
|
bors r=adityamaru |
|
Build failed: |
|
Got a nil pointer deref on |
|
bors r=adityamaru |
|
Build succeeded: |
This commit ensures that producer goroutines created during the
initialization of bulk processors use the same context as bp.Ctx.
This does change the context that these goroutines were using,
but that shouldn't actually have any effects on the processors, other
than missing tracing capabilities.
Closes #60940.
Closes #60984
Closes #60983
Closes #60977
Closes #60990
Closes #60989
Closes #60979
Closes #60987
Closes #60986
Closes #60985
Closes #60981
Closes #60980
Closes #60978
Closes #60978
Closes #60976
Closes #60975
Release note: None