Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-26 Thread via GitHub


gyfora merged PR #755:
URL: https://github.com/apache/flink-kubernetes-operator/pull/755


-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-26 Thread via GitHub


gyfora commented on PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#issuecomment-1911839594

   🚒 


-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-26 Thread via GitHub


mxm commented on PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#issuecomment-1911834062

   This looks good. Any further comments @gyfora?


-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-25 Thread via GitHub


justin-chen commented on PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#issuecomment-1910444585

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


-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-25 Thread via GitHub


gyfora commented on PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#issuecomment-1909724348

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


-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-24 Thread via GitHub


justin-chen commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1465812054


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/reconciler/ReconciliationMetadata.java:
##
@@ -42,6 +42,7 @@ public class ReconciliationMetadata {
 public static ReconciliationMetadata from(AbstractFlinkResource 
resource) {
 ObjectMeta metadata = new ObjectMeta();
 metadata.setGeneration(resource.getMetadata().getGeneration());
+
resource.getStatus().setObservedGeneration(resource.getMetadata().getGeneration());

Review Comment:
   Done!



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-23 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1464380455


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/reconciler/ReconciliationMetadata.java:
##
@@ -42,6 +42,7 @@ public class ReconciliationMetadata {
 public static ReconciliationMetadata from(AbstractFlinkResource 
resource) {
 ObjectMeta metadata = new ObjectMeta();
 metadata.setGeneration(resource.getMetadata().getGeneration());
+
resource.getStatus().setObservedGeneration(resource.getMetadata().getGeneration());

Review Comment:
   I would move this code to:
   `ReconciliationStatus#serializeAndSetLastReconciledSpec` and 
`ReconciliationUtils#updateReconciliationMetadata` that way it's a bit more 
intuitive when the setting of the generation happens. It's a bit unexpected 
that a constructor method for `ReconciliationMetadata` mutates the resources 
here.
   
   I know that my suggestion will actually duplicate the setting of the 
observedGeneration in the status but I think it will be cleaner once we remove 
the now deprecated meta generation field :) 



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-23 Thread via GitHub


justin-chen commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1463969344


##
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconciliationUtils.java:
##
@@ -126,6 +126,9 @@ private static  void 
updateStatusForSpecReconcil
 status.setError(null);
 
reconciliationStatus.setReconciliationTimestamp(clock.instant().toEpochMilli());
 
+// Set observedGeneration
+status.setObservedGeneration(target.getMetadata().getGeneration());

Review Comment:
   @gyfora I see, thanks for the review. I moved the setting of 
`observedGeneration` to be at the same time/place as 
`lastReconciledSpec.meta.generation`, and replaced the internal usage of the 
latter in two locations. Tests pass, however please advise if there is 
somewhere I may have overlooked.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-21 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1461096099


##
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconciliationUtils.java:
##
@@ -126,6 +126,9 @@ private static  void 
updateStatusForSpecReconcil
 status.setError(null);
 
reconciliationStatus.setReconciliationTimestamp(clock.instant().toEpochMilli());
 
+// Set observedGeneration
+status.setObservedGeneration(target.getMetadata().getGeneration());

Review Comment:
   I still don't think that this is consistent with the current content of the 
lastReconciledSpec metadata. It's not about setting it to the same value 
(source of truth) it's also about setting it at the exact same time.
   
   Also we should remove internal useges of the 
lastReconciledSpec.meta.generation and replace it with this one. We have tests 
for the behaviour so that would validate it.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-18 Thread via GitHub


justin-chen commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1458380663


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   πŸ‘ I have updated the PR to set `status.observedGeneration` using the [same 
source of 
truth](https://github.com/apache/flink-kubernetes-operator/blob/main/flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/reconciler/ReconciliationMetadata.java#L44)
 as 
`status.reconciliationStatus.lastReconciledSpec.resource_metadata.generation`, 
such that we can later remove the latter field without the `observedGeneration` 
depending on it.
   
   The new field is set in `updateStatusForSpecReconciliation`  method 
alongside the `lastReconciledSpec`.
   
   



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-18 Thread via GitHub


mxm commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1457361368


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   In essence, the current code already reflected the desired functionality, we 
just need to move the code to set the top-level generation id to where we 
initially set it. I hope that's easily doable. If not, let us know.



##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   In essence, the current code already reflects the desired functionality, we 
just need to move the code to set the top-level generation id to where we 
initially set it. I hope that's easily doable. If not, let us know.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-18 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1457356337


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   I think you misunderstand. Let's leave the `generation` field in the 
lastReconciledSpec where it is now for backward compatibility. 
   
   However instead of getting it from there and setting it in the top level 
status, set it in the top level status in the same step where we set in the 
lastReconciledSpec. That way the next person doesn't have to rewrite the entire 
logic to remove the deprecated meta field from lastReconciledSpec.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-17 Thread via GitHub


justin-chen commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1456093406


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   > we should set the new field next to where the old one is set so that the 
old logic is easy to remove later
   
   @gyfora Not sure I understand, is the intention to duplicate the 
`observedGeneration` such that it's available via both 
`status.observedGeneration` (we need it at this top level) and 
`status.reconciliationStatus.lastReconciledSpec.resource_metadata`?
   
   Given the status currently looks like this (with my current PR):
   ```
   status:
   ...
   observedGeneration: 123
   reconciliationStatus:
   lastReconciledSpec: '{
   "spec": {
   ...
   },
   "resource_metadata": {
   "apiVersion": "flink.apache.org/v1beta1",
   "metadata": {   // io.fabric8.kubernetes.api.model.ObjectMeta
   "generation": 123
   },
   "firstDeployment": false
   }
   }'
   lastStableSpec: '{...}'
   reconciliationTimestamp:
   state: DEPLOYED
   ```



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-16 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1453254195


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   And we should add a note of deprecation and a jira to track it. Although we 
don’t have any explicit guarantees here for the status especially for contents 
of json fields 



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-16 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1453252121


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   In that case we should set the new field next to where the old one is set so 
that the old logic is easy to remove later .



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-16 Thread via GitHub


mxm commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1453245131


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   I would prefer to keep the field in lastReconciledSpec to avoid downstream 
breakage. At least for the next release.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452979030


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   @justin-chen I would prefer to replace the implementation and call it out in 
the release notes instead. That way we end up with a cleaner codebase.
   
   Any concerns from your end @mxm ?



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


justin-chen commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452942179


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   Thanks for the guidance - I've added `observedGeneration` to `CommonStatus` 
class and set it alongside the `lastReconciledSpec`. I've left the `generation` 
within the `lastReconciledSpec` intact in case there are existing client 
dependencies on it. Let me know about any additional changes to be made



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452529625


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   I agree that some tools may rely on this, so I am +1 on the effort in 
general. But there is no point in adding a meaningless new field



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


ryanvanhuuksloot commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452524915


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   >We do have this information available already, via the lastReconciledSpec 
field. Not sure whether we need to expose this as a top-level field.
   
   The idea is that the Kubernetes spec expects that this status field exists 
in this particular place. This type of PR allows for 3rd party tools to work 
well with the Flink Operator. (ie. our build tools internally now can 
communicate with the operator which is πŸ™)
   
   However, agreed with Gyula on how it should replace semantically what exists 
today.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


gyfora commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452334612


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   Exactly as @mxm said. 
   
   Also the field inside the last reconciled spec has a slightly different and 
more correct semantics. I think what you did here (update the observed 
generation) doesn't make too much practical sense as the operator may fail 
before even seeing the spec. 
   
   I see the value of having a top level `observedGeneration` field but it 
should replace the current field hidden inside the lastReconciledSpec with the 
same semantics.



-- 
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-unsubscr...@flink.apache.org

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



Re: [PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


mxm commented on code in PR #755:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/755#discussion_r1452280124


##
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkDeploymentStatus.java:
##
@@ -55,4 +55,7 @@ public class FlinkDeploymentStatus extends 
CommonStatus {
 
 /** Information about the TaskManagers for the scale subresource. */
 private TaskManagerInfo taskManager;
+
+/** Last observed generation of the FlinkDeployment. */
+private Long observedGeneration;

Review Comment:
   We do have this information available already, via the `lastReconciledSpec` 
field. Not sure whether we need to expose this as a top-level field.
   
   CC @gyfora 



-- 
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-unsubscr...@flink.apache.org

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



[PR] [FLINK-33803] Set observedGeneration at end of reconciliation [flink-kubernetes-operator]

2024-01-15 Thread via GitHub


justin-chen opened a new pull request, #755:
URL: https://github.com/apache/flink-kubernetes-operator/pull/755

   
   
   ## 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:
 - Incremental `observedGeneration` increases due to spec changes
 - Unchanged `observedGeneration` for rollback due to validation error
 - Unchanged `observedGeneration` for unchanged spec
   
   ## 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
   


-- 
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-unsubscr...@flink.apache.org

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