-
Notifications
You must be signed in to change notification settings - Fork 2
refactor(status): aynsc update status #151
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
Conversation
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2025-05-23T06:44:48Z"
gatewayAPIChannel: standard
gatewayAPIVersion: v1.2.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: partial
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 0
Passed: 32
Skipped: 1
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 1 test skips. |
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.
Pull Request Overview
This PR refactors status updates to be asynchronous by introducing a dedicated status handler and replacing direct Status().Update calls with non‐blocking enqueues.
- Added a new
statuspackage withUpdateHandler,Updaterinterface, andUpdateWriterfor async status updates. - Updated manager setup to register the
UpdateHandlerand pass its writer to controllers. - Refactored controllers to gather
status.Updateentries inTranslateContextand enqueue them viaUpdater.Update.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/provider.go | Switched TranslateContext.StatusUpdaters from []client.Object to []status.Update. |
| internal/manager/run.go | Registered NewStatusUpdateHandler with the manager and added its writer. |
| internal/manager/controllers.go | Extended setupControllers to accept status.Updater and wired it to most reconcilers. |
| internal/controller/status/updater.go | Introduced async UpdateHandler, Updater interface, and UpdateWriter. |
| internal/controller/utils.go | Added NamespacedName helper for status updates. |
| internal/controller/*.go | Replaced sync Status().Update calls with enqueues to Updater.Update. |
Comments suppressed due to low confidence (2)
internal/manager/controllers.go:101
- The IngressClassReconciler is instantiated without the Updater field, which prevents async status updates for IngressClass. Add
Updater: updater,to this struct literal.
Provider: pro,
internal/controller/status/updater.go:1
- [nitpick] No unit tests were added for the new
UpdateHandler; consider adding tests for itsStart,apply, andWriterbehaviors.
// Licensed under the Apache License, Version 2.0 (the "License");
| } | ||
| if updated { | ||
| tctx.StatusUpdaters = append(tctx.StatusUpdaters, policy.DeepCopy()) | ||
| tctx.StatusUpdaters = append(tctx.StatusUpdaters, status.Update{ |
Copilot
AI
May 23, 2025
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.
[nitpick] The repeated status.Update closures across controllers introduce duplication; consider extracting a helper function to build these updates.
ronething
left a comment
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.
lgtm
To avoid state updates blocking the normal synchronization of resources in scenarios where a large amount of resources are synchronized.
Type of change:
What this PR does / why we need it:
Pre-submission checklist: