-
Notifications
You must be signed in to change notification settings - Fork 0
feat(flowcontrol): expose round-robin fairness identity via DumpState #5
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,8 @@ type roundRobin struct { | |
| name string | ||
| } | ||
|
|
||
| var _ fwkplugin.StateDumper = &roundRobin{} | ||
|
|
||
| func newRoundRobin(name string) *roundRobin { | ||
| if name == "" { | ||
| name = RoundRobinFairnessPolicyType | ||
|
|
@@ -126,3 +128,27 @@ func (p *roundRobin) Pick( | |
| c.lastSelected = nil | ||
| return nil, nil //nolint:nilnil | ||
| } | ||
|
|
||
| // roundRobinState is the debug snapshot for the round-robin fairness policy. | ||
| // The policy is a stateless singleton: its per-priority-band cursors live in the | ||
| // flow-control registry (PriorityBandAccessor.PolicyState), not on the plugin, so | ||
| // no live scheduling state is reachable from here. This snapshot reports only | ||
| // static identity plus an explicit marker so operators are not misled into | ||
| // thinking the cursor position is shown. | ||
| type roundRobinState struct { | ||
| Policy string `json:"policy"` | ||
| Stateful bool `json:"stateful"` | ||
| Note string `json:"note"` | ||
| } | ||
|
|
||
| // DumpState implements [fwkplugin.StateDumper]. The round-robin policy holds no | ||
| // mutable runtime state on the plugin; the per-band cursors are owned by the flow | ||
| // registry. The payload is therefore a fixed, bounded struct independent of band | ||
| // or flow cardinality. | ||
| func (p *roundRobin) DumpState() (json.RawMessage, error) { | ||
|
Comment on lines
+145
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Condense this method comment to only non-obvious intent and avoid repeating details already stated in nearby docs and payload text. [custom_rule] Severity Level: Minor Why it matters? 🤔This comment largely repeats the same explanation already given in the surrounding struct doc and the Note field, rather than adding distinct non-obvious rationale. That fits the rule against comments that merely restate code or duplicate nearby documentation, so it is verified. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go
**Line:** 145:148
**Comment:**
*Custom Rule: Condense this method comment to only non-obvious intent and avoid repeating details already stated in nearby docs and payload text.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return json.Marshal(roundRobinState{ | ||
| Policy: RoundRobinFairnessPolicyType, | ||
| Stateful: false, | ||
| Note: "round-robin cursors are tracked per priority band in the flow-control registry, not on this plugin", | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ package roundrobin | |
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "sync" | ||
| "sync/atomic" | ||
|
|
@@ -282,3 +283,88 @@ func TestRoundRobin_Pick_Concurrency(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestRoundRobin_DumpState(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // decode unmarshals the payload into the snapshot shape plus a catch-all so a | ||
| // stray extra field (e.g. a leaked flow ID or name) would be detectable. | ||
| decode := func(t *testing.T, payload json.RawMessage) (roundRobinState, map[string]json.RawMessage) { | ||
| t.Helper() | ||
| require.True(t, json.Valid(payload), "DumpState must return valid JSON") | ||
| var s roundRobinState | ||
| require.NoError(t, json.Unmarshal(payload, &s)) | ||
| var raw map[string]json.RawMessage | ||
| require.NoError(t, json.Unmarshal(payload, &raw)) | ||
| return s, raw | ||
| } | ||
|
|
||
| assertHonest := func(t *testing.T, s roundRobinState, raw map[string]json.RawMessage) { | ||
| t.Helper() | ||
| assert.Equal(t, RoundRobinFairnessPolicyType, s.Policy) | ||
| assert.False(t, s.Stateful, "policy holds no reachable mutable state") | ||
| assert.NotEmpty(t, s.Note, "note must explain where cursors actually live") | ||
| // Exactly the three sanitized fields, nothing else (no name, no flow IDs). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Consolidate this repeated fact into one canonical place (either the key set/assertion message or the comment) to avoid duplicating the same payload-field contract in multiple forms. [custom_rule] Severity Level: Minor Why it matters? 🤔The comment repeats the same payload-shape contract that is already encoded immediately below in (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin_test.go
**Line:** 307:307
**Comment:**
*Custom Rule: Consolidate this repeated fact into one canonical place (either the key set/assertion message or the comment) to avoid duplicating the same payload-field contract in multiple forms.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| wantKeys := map[string]bool{"policy": true, "stateful": true, "note": true} | ||
| for k := range raw { | ||
| assert.Truef(t, wantKeys[k], "unexpected key %q in DumpState payload", k) | ||
| } | ||
| assert.Len(t, raw, len(wantKeys), "payload must contain only policy, stateful, note") | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| policy *roundRobin | ||
| }{ | ||
| {name: "default name", policy: newRoundRobin("")}, | ||
| {name: "custom name", policy: newRoundRobin("rr-a")}, | ||
| } | ||
|
|
||
| var defaultBytes json.RawMessage | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| payload, err := tt.policy.DumpState() | ||
| require.NoError(t, err) | ||
| s, raw := decode(t, payload) | ||
| assertHonest(t, s, raw) | ||
| if tt.name == "default name" { | ||
| defaultBytes = payload | ||
| } else { | ||
| // Name is not emitted: a differently-named policy yields identical bytes. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Remove this comment or replace it with non-obvious rationale, since it currently just narrates what the immediately following assertion already shows. [custom_rule] Severity Level: Minor Why it matters? 🤔The comment simply paraphrases the immediately following assertion that compares the bytes for two differently named policies. It does not add non-obvious rationale, so it matches the rule against comments that restate code. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin_test.go
**Line:** 333:333
**Comment:**
*Custom Rule: Remove this comment or replace it with non-obvious rationale, since it currently just narrates what the immediately following assertion already shows.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| assert.JSONEq(t, string(defaultBytes), string(payload), | ||
| "plugin name must not appear in the dump") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // Plugin-specific edge case: the dump cannot reflect cursor movement because | ||
| // the cursor lives in the band's PolicyState, not on the plugin. Advance a | ||
| // cursor via Pick, then assert DumpState is byte-identical to the untouched case. | ||
| t.Run("invariant after Pick", func(t *testing.T) { | ||
| policy := newRoundRobin("") | ||
| ctx := context.Background() | ||
| state := policy.NewState(ctx) | ||
| queue1 := &fwkfcmocks.MockFlowQueueAccessor{LenV: 1, FlowKeyV: flow1Key} | ||
| queue2 := &fwkfcmocks.MockFlowQueueAccessor{LenV: 2, FlowKeyV: flow2Key} | ||
| mockBand := &fwkfcmocks.MockPriorityBandAccessor{ | ||
| PolicyStateV: state, | ||
| FlowKeysFunc: func() []flowcontrol.FlowKey { return []flowcontrol.FlowKey{flow1Key, flow2Key} }, | ||
| QueueFunc: func(id string) flowcontrol.FlowQueueAccessor { | ||
| if id == flow1ID { | ||
| return queue1 | ||
| } | ||
| return queue2 | ||
| }, | ||
| } | ||
| selected, err := policy.Pick(ctx, mockBand) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, selected, "Pick should advance the band cursor") | ||
|
|
||
| payload, err := policy.DumpState() | ||
| require.NoError(t, err) | ||
| s, raw := decode(t, payload) | ||
| assertHonest(t, s, raw) | ||
| assert.JSONEq(t, string(defaultBytes), string(payload), | ||
| "DumpState must not and cannot reflect cursor movement") | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Remove or significantly shorten this struct doc block and keep the detailed state-location explanation in only one canonical place to avoid repeating the same fact multiple times. [custom_rule]
Severity Level: Minor⚠️
Why it matters? 🤔
The file repeats the same factual point about cursor state living in the flow-control registry rather than on the plugin in multiple places, including this struct doc, the DumpState comment, and the JSON note field. That matches the rule against duplicated factual content across docs/comments/examples, so the suggestion is verified.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖