-
Notifications
You must be signed in to change notification settings - Fork 611
api: Update gateway status to include AttachedListeners
#4211
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?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: davidjumani 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 |
AttachedListenersAttachedListeners
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.
I am going to take a contrarian point of view here and say maybe we shouldn't add this. AttachedRoutes, IMO, is an extremely high-cost status field to update with extremely little value.
Our implementation spends like 20% of its CPU calculating AttachedRoutes...
If we do decide to add it though, the PR itself LGTM
|
@howardjohn regardless of the implementation detail (which I see your point and agree about the performance caveat), I am wondering that we need a way to let the Gateway owner know that there are some ListenerSets attached to the Gateway. Do you suggest some different approach for it? |
|
A FWIW the implementation detail is not specific to any implementation, it necessarily at least doubles the number of status writes to the API server on a route creation/deletion. |
|
ok, seems reasonable. @robscott @youngnick for some comments, we can change for a boolean instead. I think it is important for an admin at least to know something is attached to the gateway |
Thanks @howardjohn - The |
What type of PR is this?
Add one of the following kinds:
/kind gep
What this PR does / why we need it:
Updates the status of the Gateway to include AttachedListeners which is the count of successful ListenerSet attachments to the gateway. Ref: slack thread
Does this PR introduce a user-facing change?: