Skip to content

router: support random value for cluster specifier plugin #39979

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Jun 21, 2025

Commit Message: router: support random value for cluster specifier plugin
Additional Description:

Now, the cluster specifier plugin could also use the random value to select cluster.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

wbpcode added 2 commits June 21, 2025 05:48
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode wbpcode requested a review from agrawroh as a code owner June 21, 2025 06:08
wbpcode added 2 commits June 21, 2025 12:49
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode
Copy link
Member Author

wbpcode commented Jun 22, 2025

/retest

@wbpcode
Copy link
Member Author

wbpcode commented Jun 23, 2025

Hi, @agrawroh, could you take a quick look at this when you get free time. I think you have reviewed lots of related changes. So, nag you again here. 🙏

@ramaraochavali
Copy link
Contributor

Curious, where is this random value used now and what is the use case? I just seeing the interface change but none of the implementations use it

@wbpcode
Copy link
Member Author

wbpcode commented Jun 23, 2025

Curious, where is this random value used now and what is the use case? I just seeing the interface change but none of the implementations use it

I will wrap the weighted cluster as an cluster specifier plugin at next PR and the random value will be used there. It will also be helpful for any custom specifier plugin where need a consitant random value as reference of cluster selection.

Copy link
Contributor

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

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

LGTM, Thank You @wbpcode!

Should we add a test for the new behavior or would it be part of the follow-up?

@agrawroh
Copy link
Contributor

Curious, where is this random value used now and what is the use case? I just seeing the interface change but none of the implementations use it

I will wrap the weighted cluster as an cluster specifier plugin at next PR and the random value will be used there. It will also be helpful for any custom specifier plugin where need a consitant random value as reference of cluster selection.

Sorry for the delay, didn't get enough time on Friday.

@wbpcode
Copy link
Member Author

wbpcode commented Jun 24, 2025

LGTM, Thank You @wbpcode!

Should we add a test for the new behavior or would it be part of the follow-up?

I should be covered by the weighted cluster PR.

@wbpcode
Copy link
Member Author

wbpcode commented Jun 24, 2025

/assign-from @envoyproxy/senior-maintainers ping for an approval from senior maintainer for the core code change.

Copy link

@envoyproxy/senior-maintainers assignee is @yanavlasov
ping assignee is @None
for assignee is @None
an assignee is @None
approval assignee is @None
from assignee is @None
senior assignee is @None
maintainer assignee is @None
for assignee is @None
the assignee is @None
core assignee is @None
code assignee is @None
change. assignee is @None

🐱

Caused by: a #39979 (comment) was created by @wbpcode.

see: more, trace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants