Skip to content

Commit d9c2a9f

Browse files
committed
chore(flowcontrol): address review feedback
- fallbackRequest: document that on the fallback path item.OriginalRequest().FlowKey() reports the fallback priority rather than the originally requested one, despite the method name. Note surfacing the original priority in metrics as a follow-up. - Drop the redundant NotErrorIs assertion in the band-GC regression test; require.NoError already fails the test on any error. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
1 parent cfc61c2 commit d9c2a9f

2 files changed

Lines changed: 7 additions & 2 deletions

File tree

pkg/epp/flowcontrol/controller/controller.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,12 @@ func (fc *FlowController) EnqueueAndWait(
282282

283283
// fallbackRequest wraps a FlowControlRequest to override its flow key, so a request that falls back to a different
284284
// priority is enqueued under the band that was actually leased rather than its original (unprovisioned) band.
285+
//
286+
// Trade-off: downstream consumers see this wrapper, so item.OriginalRequest().FlowKey() reports the fallback
287+
// priority rather than the requested one — despite the method name. This is intentional, since the item must be
288+
// leased, distributed, and enqueued consistently at the fallback priority. The originally requested priority
289+
// therefore survives only in the withConnectionWithFallback log; surfacing it in metrics is left as a follow-up
290+
// (a dedicated fallback counter labeled with the original priority).
285291
type fallbackRequest struct {
286292
flowcontrol.FlowControlRequest
287293
key flowcontrol.FlowKey

pkg/epp/flowcontrol/registry/registry_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -957,11 +957,10 @@ func TestFlowRegistry_PriorityBandGarbageCollection(t *testing.T) {
957957
require.True(t, exists,
958958
"Control-plane-desired band %d must survive GC after inactivity", desiredPrio)
959959

960-
// A follow-up request to the still-desired band must not be rejected/demoted.
960+
// A follow-up request to the still-desired band must not be rejected with ErrPriorityBandNotFound.
961961
err := h.fr.WithConnection(key, func(conn contracts.ActiveFlowConnection) error { return nil })
962962
require.NoError(t, err,
963963
"Request to a still-desired band must succeed after inactivity")
964-
assert.NotErrorIs(t, err, contracts.ErrPriorityBandNotFound)
965964
})
966965

967966
t.Run("ShouldCollectMultipleBands_InOneCycle", func(t *testing.T) {

0 commit comments

Comments
 (0)