Skip to content

[Dataflow Streaming] Reduce contention on CommitQueue - #39737

Open
arunpandianp wants to merge 1 commit into
apache:masterfrom
arunpandianp:commitQueue
Open

[Dataflow Streaming] Reduce contention on CommitQueue#39737
arunpandianp wants to merge 1 commit into
apache:masterfrom
arunpandianp:commitQueue

Conversation

@arunpandianp

@arunpandianp arunpandianp commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Replaces LinkedBlockingQueue with a ConcurrentLinkedQueue and a Semaphore. The harness threads don't block on the queue anymore.

Improves the internal reshuffle benchmark run time by ~10%.

#33578

Replaces LinkedBlockingQueue with a ConcurrentLinkedQueue and a
Semaphore. The harness threads don't block on the queue anymore.

Improves the internal reshuffle benchmark run time by ~10%.
@arunpandianp

Copy link
Copy Markdown
Contributor Author

R: @scwhittle

@github-actions

Copy link
Copy Markdown
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

@bvolpato

Copy link
Copy Markdown
Contributor

Is delayed visibility acceptable here? With concurrent producers, one can pause after queue.add() but before availableItems.release(). Another producer can enqueue and release, allowing consumer to dequeue first item using second permit and leaving second item unavailable until first producer resumes. Does commit queue require each completed put() to become immediately consumable?

@arunpandianp

Copy link
Copy Markdown
Contributor Author

@bvolpato Thanks, that is a good observation. Such interleaving are possible and i think it is fine, with how the queue is used today in StreamingEngineWorkCommitter and StreamingApplianceCommitter.

It is true that a consumer can consume items using permits released after a different item. Generally the logic to stop consumers (or draining queue) should know and happen after all producer puts are complete. So that edge i think resolves the visibility problems here.

Having said that, changing to LinkedTransferQueue might be a simpler option with similar benefits. Testing it.

@scwhittle

Copy link
Copy Markdown
Contributor

LinkedTransferQueue does seem like it would be simpler, so I'd prefer that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants