You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/20 23:52:40 UTC

[GitHub] [flink-kubernetes-operator] Aitozi opened a new pull request, #176: [FLINK-27279] Extract common status interfaces

Aitozi opened a new pull request, #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176

   This pr is meant to extract common status interfaces to simplify the ReconcileUtils
   
   **The brief change log**
   
   1. Introduce the `CommonView` and `SpecView` to describe the common part between the `FlinkDeployment` and `FlinkSessionJob`
   2. Use the `ReconciliationStatus` for both `FlinkDeployment` and `FlinkSessionJob`
   


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856945232


##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   The inheritence structure breaks the (not very sophisticated) doc generation logic. We should either fix it here or open a blocker jira to track this



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/ReconciliationStatus.java:
##########
@@ -54,19 +49,20 @@ public class ReconciliationStatus {
     private ReconciliationState state = ReconciliationState.DEPLOYED;
 
     @JsonIgnore
-    public FlinkDeploymentSpec deserializeLastReconciledSpec() {
-        return ReconciliationUtils.deserializedSpecWithVersion(
-                lastReconciledSpec, FlinkDeploymentSpec.class);
+    public abstract Class<SPEC> getSpecClass();

Review Comment:
   interesting, I will take a look at this later :) 



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/SpecView.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.spec;
+
+/** The common view of the spec. */
+public interface SpecView {

Review Comment:
   Could this be an abstract class instead called `FlinkSpecBase` or `AbstractFlinkSpec` which could contain the job field (with the lombok annotation) that both deployment and sessionjobspec would extend?



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/CommonStatus.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.status;
+
+import org.apache.flink.annotation.Experimental;
+import org.apache.flink.kubernetes.operator.crd.spec.SpecView;
+
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/** Last observed common status of the Flink deployment/Flink SessionJob. */
+@Experimental
+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public abstract class CommonStatus<SPEC extends SpecView> {
+
+    /** Last observed status of the Flink job on Application/Session cluster. */
+    private JobStatus jobStatus = new JobStatus();
+
+    /** Error information about the FlinkDeployment/FlinkSessionJob. */
+    private String error;
+
+    public abstract ReconciliationStatus<SPEC> getReconciliationStatus();

Review Comment:
   Missing javadoc comment



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856933400


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconcileTarget.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.reconciler;
+
+import org.apache.flink.kubernetes.operator.crd.spec.JobSpec;
+import org.apache.flink.kubernetes.operator.crd.status.ReconciliationStatus;
+
+import javax.annotation.Nullable;
+
+/**
+ * The interface is responsible to handle the reconciliation result. For the common logic, it
+ * provides method to extract the common view between the {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkDeployment} and {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkSessionJob} to simplify the custom resource
+ * manipulation. For the special part of each custom resource, we can extend the interface to let
+ * the target custom resource react to the reconciliation result correspondingly.
+ *
+ * @param <SPEC> the common view of the custom resource getSpec
+ */
+public interface ReconcileTarget<SPEC extends ReconcileTarget.SpecView> {
+
+    /** The common view of the spec. */
+    interface SpecView {
+        JobSpec getJobSpec();
+    }
+
+    /**
+     * Get the current getSpec of the custom resource.
+     *
+     * @return the current getSpec.
+     */
+    SPEC getSpec();
+
+    /**
+     * Get the current reconciliation status.
+     *
+     * @return the current reconciliation status.
+     */
+    ReconciliationStatus<SPEC> getReconcileStatus();
+
+    /**
+     * Let the target custom resource handle the reconciliation error.
+     *
+     * @param error The error to be handled.
+     */
+    void handleError(@Nullable String error);

Review Comment:
   Good suggestion, I have refactor based on your suggestion, please take a look again.



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856142721


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconcileTarget.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.reconciler;
+
+import org.apache.flink.kubernetes.operator.crd.spec.JobSpec;
+import org.apache.flink.kubernetes.operator.crd.status.ReconciliationStatus;
+
+import javax.annotation.Nullable;
+
+/**
+ * The interface is responsible to handle the reconciliation result. For the common logic, it
+ * provides method to extract the common view between the {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkDeployment} and {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkSessionJob} to simplify the custom resource
+ * manipulation. For the special part of each custom resource, we can extend the interface to let
+ * the target custom resource react to the reconciliation result correspondingly.
+ *
+ * @param <SPEC> the common view of the custom resource getSpec
+ */
+public interface ReconcileTarget<SPEC extends ReconcileTarget.SpecView> {
+
+    /** The common view of the spec. */
+    interface SpecView {
+        JobSpec getJobSpec();
+    }
+
+    /**
+     * Get the current getSpec of the custom resource.
+     *
+     * @return the current getSpec.
+     */
+    SPEC getSpec();
+
+    /**
+     * Get the current reconciliation status.
+     *
+     * @return the current reconciliation status.
+     */
+    ReconciliationStatus<SPEC> getReconcileStatus();
+
+    /**
+     * Let the target custom resource handle the reconciliation error.
+     *
+     * @param error The error to be handled.
+     */
+    void handleError(@Nullable String error);

Review Comment:
   I think these methods should be part of the CommonStatus interface instead. Simply have the ReonciliationStatus<SPEC> and error fields there and use get/set



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857070657


##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   Get, I will try to make up the content in this way



##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   Get it, I will try to make up the content in this way



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#issuecomment-1104562068

   I also try to make the ReconcileUtils to a new Object to handle the `reconciledSucceed`, `reconcileError` and `savepointSucceed`. But I have to pass the object everywhere like `controller` and `reconcilerFactory` and `reconciler`. So I keep use the static way but unify the handle for  `FlinkDeployment` and `FlinkSessionJob`
   
   Please let me know if you have better suggestion cc @gyfora @wangyang0918 thanks


-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857045847


##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   I would open another PR after this to fix this problem soon .



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#issuecomment-1104940967

   @gyfora Thanks for your suggestion, I will try as you suggested  


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856944116


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/SpecView.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.spec;
+
+/** The common view of the spec. */
+public interface SpecView {

Review Comment:
   I think it would be better to call this SpecWithJob :) 



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857045636


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/CommonStatus.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.status;
+
+import org.apache.flink.annotation.Experimental;
+import org.apache.flink.kubernetes.operator.crd.spec.SpecView;
+
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/** Last observed common status of the Flink deployment/Flink SessionJob. */
+@Experimental
+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public abstract class CommonStatus<SPEC extends SpecView> {
+
+    /** Last observed status of the Flink job on Application/Session cluster. */
+    private JobStatus jobStatus = new JobStatus();
+
+    /** Error information about the FlinkDeployment/FlinkSessionJob. */
+    private String error;
+
+    public abstract ReconciliationStatus<SPEC> getReconciliationStatus();

Review Comment:
   fixed



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857045690


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/SpecView.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.spec;
+
+/** The common view of the spec. */
+public interface SpecView {

Review Comment:
   Nice suggestion, It looks more naturally now :).



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857045553


##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   I also confused here, what‘s the expect content here? Do we need to display the `FlinkDeploymentReconciliationStatus` fileds include (in the parent) directly and skip the abstract classes



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#issuecomment-1106331702

   Due to the java type erasure, I can't not completly eliminate the `FlinkDeploymentReconciliationStatus` and `FlinkSessionJobReconciliationStatus` because I can't get the class at runtime 😔.
   The new interface `ReconcileTarget` is responsible for get the common view of the custom resource. If we have special reconcile update logic for different custom resource, we can extend the interface to make it happen. What do you think about the current implementation @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-unsubscribe@flink.apache.org

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

[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#issuecomment-1104898624

   @Aitozi seems like the FlinkDeploymentStatus and FlinkSessionJobStatus also have a lot in common (the only diff is that FlinkDeploymentStatus has an extra JMDeploymentStatus 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-unsubscribe@flink.apache.org

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

[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#issuecomment-1106387142

   Thank you, I will check this later 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-unsubscribe@flink.apache.org

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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856144200


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconcileTarget.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.reconciler;
+
+import org.apache.flink.kubernetes.operator.crd.spec.JobSpec;
+import org.apache.flink.kubernetes.operator.crd.status.ReconciliationStatus;
+
+import javax.annotation.Nullable;
+
+/**
+ * The interface is responsible to handle the reconciliation result. For the common logic, it
+ * provides method to extract the common view between the {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkDeployment} and {@link
+ * org.apache.flink.kubernetes.operator.crd.FlinkSessionJob} to simplify the custom resource
+ * manipulation. For the special part of each custom resource, we can extend the interface to let
+ * the target custom resource react to the reconciliation result correspondingly.
+ *
+ * @param <SPEC> the common view of the custom resource getSpec
+ */
+public interface ReconcileTarget<SPEC extends ReconcileTarget.SpecView> {
+
+    /** The common view of the spec. */
+    interface SpecView {
+        JobSpec getJobSpec();
+    }
+
+    /**
+     * Get the current getSpec of the custom resource.
+     *
+     * @return the current getSpec.
+     */
+    SPEC getSpec();
+
+    /**
+     * Get the current reconciliation status.
+     *
+     * @return the current reconciliation status.
+     */
+    ReconciliationStatus<SPEC> getReconcileStatus();
+
+    /**
+     * Let the target custom resource handle the reconciliation error.
+     *
+     * @param error The error to be handled.
+     */
+    void handleError(@Nullable String error);

Review Comment:
   That way the ReconcileTerget interface could be also removed and use `CustomResource<SPEC extends SpecView, STATUS extends CommonStatus>`



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856146587


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/ReconciliationStatus.java:
##########
@@ -54,19 +49,20 @@ public class ReconciliationStatus {
     private ReconciliationState state = ReconciliationState.DEPLOYED;
 
     @JsonIgnore
-    public FlinkDeploymentSpec deserializeLastReconciledSpec() {
-        return ReconciliationUtils.deserializedSpecWithVersion(
-                lastReconciledSpec, FlinkDeploymentSpec.class);
+    public abstract Class<SPEC> getSpecClass();

Review Comment:
   You could also simply add a constructor that takes the spec class (you have that when you call initStatus by calling getSpec().getClass()). Then you would not need 2 subclasses just to implement this.



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r857070424


##########
docs/content/docs/custom-resource/reference.md:
##########
@@ -160,26 +160,41 @@ This page serves as a full reference for FlinkDeployment custom resource definit
 
 ## Status
 
+### CommonStatus
+**Class**: org.apache.flink.kubernetes.operator.crd.status.CommonStatus
+
+**Description**: Last observed common status of the Flink deployment/Flink SessionJob.
+
+| Parameter | Type | Docs |
+| ----------| ---- | ---- |
+| jobStatus | org.apache.flink.kubernetes.operator.crd.status.JobStatus | Last observed status of the Flink job on Application/Session cluster. |
+| error | java.lang.String | Error information about the FlinkDeployment/FlinkSessionJob. |

Review Comment:
   I think from the users perspective it would be best to skip any abstract classes and only show the “child/leaf” classes in the hierarchy with all fields from the superclasses . 



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora merged pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora merged PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176


-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856933715


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/ReconciliationStatus.java:
##########
@@ -54,19 +49,20 @@ public class ReconciliationStatus {
     private ReconciliationState state = ReconciliationState.DEPLOYED;
 
     @JsonIgnore
-    public FlinkDeploymentSpec deserializeLastReconciledSpec() {
-        return ReconciliationUtils.deserializedSpecWithVersion(
-                lastReconciledSpec, FlinkDeploymentSpec.class);
+    public abstract Class<SPEC> getSpecClass();

Review Comment:
   I have tried this way, But if I add a field to record the spec class in `ReconciliationStatus` (making it `@JsonIgnore` ), It will cause the [CRDGenerator stack overflow ](https://github.com/fabric8io/kubernetes-client/issues/4092). So I kept it as it now



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #176: [FLINK-27279] Extract common status interfaces

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #176:
URL: https://github.com/apache/flink-kubernetes-operator/pull/176#discussion_r856944116


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/SpecView.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.kubernetes.operator.crd.spec;
+
+/** The common view of the spec. */
+public interface SpecView {

Review Comment:
   I think it would be better to call this SpecWithJob :) 



-- 
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