Skip to content

Session affinity scorer #52

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 2 commits into
base: dev
Choose a base branch
from

Conversation

mayabar
Copy link
Collaborator

@mayabar mayabar commented Apr 24, 2025

Session affinity scorer uses information stored in datastore (map of session id to pod) to set score=1 to pod for the given session id (if exists) or 0 to all other pods

mayabar added 2 commits April 24, 2025 10:31
Signed-off-by: Maya Barnea <mayab@il.ibm.com>
Signed-off-by: Maya Barnea <mayab@il.ibm.com>
@mayabar mayabar requested a review from shmuelk April 27, 2025 05:56
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file seems like it belongs to a different PR and not to a session affinity scorer

isFirst := true

for _, pod := range pods {
if isFirst {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no reason to test isFirst on every iteration, by doing some work outside the loop:

  • set the first pod and its score as the maximum Pod and score
  • set maxScore = -math.MaxFloat64

ctx.Logger.V(logutil.DEBUG).Info(fmt.Sprintf("Selecting a pod with maximum score from %d candidates: %+v", len(pods), pods))

// select pod with maximum score, if more than one with the max score - use random pods from the list
var highestScoreTargets []types.Pod
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: not sure if it makes sense to have such long variable names in a relatively short function. I don't think we have a style guide, but Go seems to favor shorter names in smaller scopes

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.

2 participants