You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "Dennis-Mircea (via GitHub)" <gi...@apache.org> on 2026/05/25 13:13:34 UTC

[PR] [FLINK-39752] Thread EventRecorder through FlinkResourceContext instead of holding it on the factory [flink-kubernetes-operator]

Dennis-Mircea opened a new pull request, #1120:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1120

   ## What is the purpose of the change
   
   `FlinkResourceContextFactory` currently holds a single `EventRecorder` as a field, while each controller separately creates its own `EventRecorder` in `FlinkOperator.register*Controller`. This makes `FlinkService`-emitted events route through the operator-scoped recorder rather than the calling controller's recorder, producing a silent asymmetry that becomes a real bug the moment any per-instance state is added to `EventRecorder`. This PR removes the factory-held recorder and threads the recorder through `getResourceContext(...)` so each controller owns its recorder end to end.
   
   ## Brief change log
   
     - Added an `EventRecorder` field (with getter) to `FlinkResourceContext`, plumbed through `FlinkDeploymentContext` and `FlinkSessionJobContext` constructors.
     - Removed the `EventRecorder` constructor argument from `FlinkResourceContextFactory`. Added it as a third parameter to `getResourceContext(resource, josdkContext, eventRecorder)`. Changed `getFlinkService(ctx)` to read the recorder via `ctx.getEventRecorder()`.
     - Updated `FlinkOperator` so each `register*Controller` method owns its own `EventRecorder`. `registerBlueGreenController` gains one for the first time (blue-green previously had none). The shared operator-scoped `EventRecorder` field was removed.
     - Added an `EventRecorder` to `FlinkBlueGreenDeploymentController` and `BlueGreenContext`. All six `getResourceContext(...)` call sites in `BlueGreenDeploymentService` now pass `context.getEventRecorder()`.
     - Updated `FlinkDeploymentController`, `FlinkSessionJobController`, `StateSnapshotReconciler`, and `StateSnapshotObserver` to pass their own recorder on every `getResourceContext(...)` call.
     - Updated the test wiring (`OperatorTestBase`, `TestingFlinkResourceContextFactory`, the four `Testing*Controller` helpers, and the unit tests that construct contexts directly) to match the new signatures.
   
   ## Verifying this change
   
   This change is already covered by existing tests, such as `FlinkDeploymentControllerTest`, `FlinkSessionJobControllerTest`, `FlinkStateSnapshotControllerTest`, `ApplicationReconcilerTest`, `SessionJobReconcilerTest`, `StateSnapshotReconcilerTest`, `BlueGreenUtilsTest`, `NativeFlinkServiceTest`, `StandaloneFlinkServiceTest`, and `FlinkResourceContextTest`.
   
   ## Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): no
     - The public API, i.e., is any changes to the `CustomResourceDescriptors`: no
     - Core observer or reconciler logic that is regularly executed: yes (signature change to `FlinkResourceContextFactory.getResourceContext` and to the four controller constructors. Behavior is unchanged because `EventRecorder` is stateless with respect to `client` and `listeners`.)
   
   ## Documentation
   
     - Does this pull request introduce a new feature? no
     - If yes, how is the feature documented? not applicable


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [FLINK-39752] Thread EventRecorder through FlinkResourceContext instead of holding it on the factory [flink-kubernetes-operator]

Posted by "Dennis-Mircea (via GitHub)" <gi...@apache.org>.
Dennis-Mircea commented on PR #1120:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1120#issuecomment-4614401175

   @gyfora I updated the PR and JIRA with the new context. Now it's a pretty straightforward and simple change.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [FLINK-39752] Thread EventRecorder through FlinkResourceContext instead of holding it on the factory [flink-kubernetes-operator]

Posted by "Dennis-Mircea (via GitHub)" <gi...@apache.org>.
Dennis-Mircea commented on PR #1120:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1120#issuecomment-4614176127

   > Overall I don't really understand why we are moving the event record from the factory field to the getResource.
   > 
   > The eventRecorder is fixed and should be created once so there is not really a point in passing it always to getResourceContext, this is exactly the point of having the factory to avoid unnecessary object passing again and again.
   > 
   > If we want to ensure that we use a single event recorder then lets use the same in the controller that we pass to the context factory
   
   Fair point on the threading. My intent was end-to-end ownership so each controller and the `FlinkService` it drives share the same recorder, and I leaned per-controller because that was already the more common shape as part of the existing implementation: session-job and snapshot controllers each create their own `EventRecorder` (only deployment reuses the factory's, and blue-green has none), and the `StatusRecorder` is per-controller too, so pairing a per-controller `EventRecorder` alongside it felt consistent.
   
   That said, I agree with your approach. I'll keep the single `EventRecorder` on the factory and pass that same instance to every controller, dropping the `getResourceContext` / `FlinkResourceContext` plumbing. That gives one consistent recorder everywhere without passing it on every call, and cleans up the session-job and snapshot controllers that were creating their own.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [FLINK-39752] Thread EventRecorder through FlinkResourceContext instead of holding it on the factory [flink-kubernetes-operator]

Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora merged PR #1120:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1120


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org