-
Notifications
You must be signed in to change notification settings - Fork 190
refactor: Improve Flow Control queue contracts for clarity and correctness #1836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor: Improve Flow Control queue contracts for clarity and correctness #1836
Conversation
This commit refactors the `framework.SafeQueue` interface to remove error returns from methods where failure is not expected in normal operation (Add, PeekHead, PeekTail, Drain, Cleanup). The interface now relies on the following premises: - Queues are unbounded and in-memory. - OOM is a panic, not a handled error. - Internal callers are trusted not to provide nil items. - PeekHead/PeekTail return nil for empty queues. This change simplifies the interface and reduces unnecessary error checking on the hot path. This commit updates the queue implementations (ListQueue, MaxMinHeap), mocks, and all callers within the `pkg/epp/flowcontrol/framework/...` tree to conform to the new interface. A subsequent commit will address callers outside this tree.
This commit refactors the `ManagedQueue` contract after the preceding commit which altered the `SafeQueue` contract. Motivation: The preceding commit made the generic `framework.SafeQueue.Add` method infallible. This created a design conflict, as the higher-level `ManagedQueue` decorator requires a *fallible* `Add` operation to atomically reject requests when its parent shard is draining. This commit addresses this by refactoring `ManagedQueue` to favor composition over embedding: - `ManagedQueue` no longer embeds `SafeQueue`; it now contains it as a field, correctly modeling the "has-a" relationship. - The `ManagedQueue.Add` method now has its own explicit, fallible contract, returning `contracts.ErrShardDraining`. - The `Remove` and `Cleanup` operations follow the new, simpler `SafeQueue` contract. This change also includes the necessary updates to all callers in the `controller` and `registry` packages to conform to these new, more robust contracts as well as updating tests and mocks.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LukeAVanDrie The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @LukeAVanDrie. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/ok-to-test |
|
This is a fairly large diffbase. The core change is simply removing error return values that can never be reached in practice from the plugin contract. The bulk of the diff is modifying all call sites of the updated methods below and removing error handling logic.
|
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR simplifies the core queuing contracts within the Flow Control framework.
This is achieved through two primary, related changes:
1. Simplifying the
SafeQueueContractThe
SafeQueueinterface and its implementations are designed for unbounded, in-memory, synchronous flow control. This context allows for a much simpler, more idiomatic, and higher-performance contract:Add,Peek*,Drain,Cleanup) cannot logically fail, as OOM conditions are panics, and capacity is managed externally. Theirerrorreturns have been removed.PeekHeadandPeekTailnow returnnilwhen a queue is empty, which is more idiomatic Go than returningErrQueueEmpty.RemoveRemains Fallible: As it deals with externally provided handles,Removeremains the only fallible operation.This refactoring removes unnecessary defensive checks and error handling from the hot path, simplifying both the queue implementations and the caller logic in the
ShardProcessor.2. Ensuring Correctness via Composition (
ManagedQueue)As part of simplifying the
SafeQueueinterface to use an infallibleAdd()method, a potential design conflict was identified. The higher-levelManagedQueuecomponent requires a fallibleAddoperation to atomically reject requests when its parent shard isDRAINING.To address this, this PR refactors
ManagedQueueto favor composition over embedding:ManagedQueueno longer embedsSafeQueue; it contains it as a field, delegating to it accordingly.Addmethod maintains its own explicit, fallible contract (-> error), allowing it to perform theisDraining()check and the addition to the underlying queue as a single, atomic, lock-protected operation.CleanupandDrainmethods follow the simplifiedSafeQueuecontract.Specific Changes:
contracts.ManagedQueue:framework.SafeQueue; it now contains it via composition.Add(item)still returns anerrorto atomically handle theErrShardDrainingcase whileCleanupandDraintake on the simplifiedSafeQueuecontract.framework.SafeQueue:Add(item)now returns no error.PeekHead()andPeekTail()now return(types.QueueItemAccessor)andnilif empty.Drain()andCleanup()now return([]types.QueueItemAccessor).framework.ErrQueueEmpty: Removed.controller,registry) have been updated to conform to these new, more robust contracts in addition to tests and mocks.Which issue(s) this PR fixes:
Tracks #1794
Does this PR introduce a user-facing change?: