Skip to content

changefeedccl: use correct channel size in parallelio#118476

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
jayshrivastava:pubsub-parallelio
Jan 30, 2024
Merged

changefeedccl: use correct channel size in parallelio#118476
craig[bot] merged 1 commit intocockroachdb:masterfrom
jayshrivastava:pubsub-parallelio

Conversation

@jayshrivastava
Copy link
Copy Markdown
Contributor

@jayshrivastava jayshrivastava commented Jan 30, 2024

Previously, the channels used for sending requests and receiving results were too small. This meant that a caller could block on sending a request even after acquiring quota. This change ensures that the size of the channels is large enough so that this blocking does not occur.

Closes: #118463
Closes: #118462
Closes: #118461
Closes: #118460
Closes: #118459
Closes: #118458
Epic: none

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Jan 30, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jan 30, 2024

This change is Reviewable

@jayshrivastava jayshrivastava marked this pull request as ready for review January 30, 2024 15:19
@jayshrivastava jayshrivastava requested a review from a team as a code owner January 30, 2024 15:19
@jayshrivastava jayshrivastava requested review from miretskiy and rharding6373 and removed request for a team January 30, 2024 15:19
@jayshrivastava
Copy link
Copy Markdown
Contributor Author

jayshrivastava commented Jan 30, 2024

I stress tested TestChangefeedEndTimeWithCursor. It would fail consistently before this change. Now --stress passes.

rharding6373
rharding6373 approved these changes Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Good find, thanks for the quick fix! I have a question about memory accounting

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jayshrivastava and @miretskiy)

pkg/ccl/changefeedccl/parallel_io.go line 97 at r1 (raw file):

		// NB: The size of these channels should not be less than the quota. This prevents the producer from
		// blocking on sending requests which have been admitted.
		requestCh: make(chan AdmittedIORequest, quota),

Do we do memory accounting for the channel size?

miretskiy
miretskiy reviewed Jan 30, 2024
View reviewed changes
jayshrivastava
jayshrivastava commented Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @rharding6373)

pkg/ccl/changefeedccl/parallel_io.go line 97 at r1 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Do we do memory accounting for the channel size?

We don't need to memory account because the requests are batches which contain cdc events and these cdc events are accounted for. The overhead is considered negligible. They get freed by batch.alloc.Release after the result comes back:

cockroach/pkg/ccl/changefeedccl/batching_sink.go

Lines 346 to 367 in cb674c2

handleResult := func(result IOResult) {
req, err := result.Consume()
batch, _ := req.(*sinkBatch)
if err != nil {
s.handleError(err)
} else {
s.metrics.recordEmittedBatch(
batch.bufferTime, batch.numMessages, batch.mvcc, batch.numKVBytes, sinkDoesNotCompress,
)
}
inflight -= batch.numMessages
if (err != nil || inflight == 0) && sinkFlushWaiter != nil {
close(sinkFlushWaiter)
sinkFlushWaiter = nil
}
batch.alloc.Release(ctx)
freeSinkBatchEvent(batch)
}

Event allocs are created here here

cockroach/pkg/ccl/changefeedccl/kvevent/blocking_buffer.go

Lines 271 to 275 in 3595935

alloc, err := b.AcquireMemory(ctx, n)
if err != nil {
return err
}
e.alloc = alloc

@jayshrivastava
changefeedccl: use correct channel size in parallelio
b5799ba 
Previously, the channels used for sending requests and receiving results were
too small. This meant that a caller could block on sending a request even
after acquiring quota. This change ensures that the size of the channels
is large enough so that this blocking does not occur.

Closes: cockroachdb#118463
Closes: cockroachdb#118462
Closes: cockroachdb#118461
Closes: cockroachdb#118460
Closes: cockroachdb#118459
Closes: cockroachdb#118458
Epic: none
@jayshrivastava
Copy link
Copy Markdown
Contributor Author

jayshrivastava commented Jan 30, 2024

bors r+

@craig craig bot merged commit 38dd16a into cockroachdb:master Jan 30, 2024
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 30, 2024

Build succeeded:

@wenyihu6 wenyihu6 mentioned this pull request Oct 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rharding6373 rharding6373 rharding6373 approved these changes

+1 more reviewer

@miretskiy miretskiy miretskiy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@jayshrivastava @cockroach-teamcity @rharding6373 @miretskiy