refactor(status): aynsc update status#151
Merged
Merged
Conversation
Contributor
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. |
Contributor
There was a problem hiding this comment.
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{ |
There was a problem hiding this comment.
[nitpick] The repeated status.Update closures across controllers introduce duplication; consider extracting a helper function to build these updates.
ronething
reviewed
May 23, 2025
ronething
reviewed
May 23, 2025
ronething
reviewed
May 23, 2025
ronething
reviewed
May 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: