Skip to content
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

[FLINK-33803] Set observedGeneration at end of reconciliation #755

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

justin-chen
Copy link
Contributor

@justin-chen justin-chen commented Jan 15, 2024

What is the purpose of the change

https://issues.apache.org/jira/browse/FLINK-33803

This pull requests adds the observedGeneration field to the FlinkDeployment CRD status. This field is used to persist the generation that was last acted on by the operator. At the end of a successful reconciliation (reconcile, RecoveryFailureException, DeploymentFailedException ), the operator sets the observedGeneration field to the current generation number (from the CRD metadata).

Brief change log

  • Operator sets observedGeneration field within FlinkDeploymentStatus after a completed reconciliation

Verifying this change

This change added tests and can be verified as follows:

  • Test setting observedGeneration based on generation metadata taken from lastReconciledSpec

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: yes
  • Core observer or reconciler logic that is regularly executed: yes

Documentation

  • Does this pull request introduce a new feature? No
  • If yes, how is the feature documented? Docs

Copy link
Contributor

@gyfora gyfora left a comment

Choose a reason for hiding this comment

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

Please see the comment about adopting the already existing observed generation logic from lastReconciledSpec and simply moving it into a top level field.

Also this change should be general to both FlinkDeployments and FlinkSessionJobs.

@justin-chen justin-chen force-pushed the FLINK-33803 branch 3 times, most recently from f253d34 to d1b278b Compare January 19, 2024 05:43
@justin-chen justin-chen marked this pull request as ready for review January 19, 2024 05:48
Copy link
Contributor

@gyfora gyfora left a comment

Choose a reason for hiding this comment

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

The change looks good overall and thanks for updating the existing usage 👍

I added a small comment to where we should set the generation so that it's a bit more intuitive when reading the code but that's an easy change.

@gyfora
Copy link
Contributor

gyfora commented Jan 25, 2024

Please generate the docs via 'mvn clean install -DskipTests -Pgenerate-docs'

@justin-chen
Copy link
Contributor Author

Updated docs with mvn clean install -DskipTests -Pgenerate-docs

@mxm
Copy link
Contributor

mxm commented Jan 26, 2024

This looks good. Any further comments @gyfora?

@gyfora
Copy link
Contributor

gyfora commented Jan 26, 2024

🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants