[FLINK-27279] Extract common status interfaces #176

Aitozi · 2022-04-20T23:52:39Z

This pr is meant to extract common status interfaces to simplify the ReconcileUtils

The brief change log

Introduce the CommonView and SpecView to describe the common part between the FlinkDeployment and FlinkSessionJob
Use the ReconciliationStatus for both FlinkDeployment and FlinkSessionJob

Verified by the current tests.

Aitozi · 2022-04-20T23:56:34Z

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

gyfora · 2022-04-21T08:52:10Z

@Aitozi seems like the FlinkDeploymentStatus and FlinkSessionJobStatus also have a lot in common (the only diff is that FlinkDeploymentStatus has an extra JMDeploymentStatus field)

but jobStatus, error and reconciliationStatus could be completely shared

This could also simplify what you have tried to do with the CommonView logic which is a little confusing right now I think

Aitozi · 2022-04-21T09:23:08Z

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

Aitozi · 2022-04-22T10:09:48Z

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

gyfora · 2022-04-22T10:56:30Z

Thank you, I will check this later today :)

gyfora · 2022-04-22T11:34:22Z

...-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconcileTarget.java

+    /**
+     * 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);


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

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

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

gyfora · 2022-04-22T11:40:23Z

...ator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/ReconciliationStatus.java

-    public FlinkDeploymentSpec deserializeLastReconciledSpec() {
-        return ReconciliationUtils.deserializedSpecWithVersion(
-                lastReconciledSpec, FlinkDeploymentSpec.class);
+    public abstract Class<SPEC> getSpecClass();


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.

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 . So I kept it as it now

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

gyfora

Added some last minor comments. Thanks @Aitozi it looks really good now.

gyfora · 2022-04-23T17:39:45Z

...tes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/CommonStatus.java

+    /** Error information about the FlinkDeployment/FlinkSessionJob. */
+    private String error;
+
+    public abstract ReconciliationStatus<SPEC> getReconciliationStatus();


Missing javadoc comment

gyfora · 2022-04-23T17:49:39Z

...ubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/SpecView.java

+package org.apache.flink.kubernetes.operator.crd.spec;
+
+/** The common view of the spec. */
+public interface SpecView {


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?

Nice suggestion, It looks more naturally now :).

gyfora · 2022-04-23T17:50:17Z

...ator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/ReconciliationStatus.java

-    public FlinkDeploymentSpec deserializeLastReconciledSpec() {
-        return ReconciliationUtils.deserializedSpecWithVersion(
-                lastReconciledSpec, FlinkDeploymentSpec.class);
+    public abstract Class<SPEC> getSpecClass();


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

gyfora · 2022-04-23T17:51:49Z

docs/content/docs/custom-resource/reference.md

+### 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. |


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

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

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

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 .

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

Aitozi force-pushed the FLINK-27279 branch from 80ef54e to 96b22c4 Compare April 22, 2022 06:20

Aitozi marked this pull request as draft April 22, 2022 06:32

Aitozi force-pushed the FLINK-27279 branch 4 times, most recently from 1f106d6 to b8cedd1 Compare April 22, 2022 08:55

[FLINK-27279] Extract common status interfaces

714a3b0

Aitozi force-pushed the FLINK-27279 branch from b8cedd1 to 714a3b0 Compare April 22, 2022 09:33

Aitozi marked this pull request as ready for review April 22, 2022 10:09

gyfora reviewed Apr 22, 2022

View reviewed changes

Aitozi force-pushed the FLINK-27279 branch 2 times, most recently from 5450276 to f62350d Compare April 23, 2022 15:11

fix commentst

3736763

Aitozi force-pushed the FLINK-27279 branch from f62350d to 3736763 Compare April 23, 2022 15:18

gyfora reviewed Apr 23, 2022

View reviewed changes

fix comments

6e077c6

gyfora merged commit f520adf into apache:main Apr 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-27279] Extract common status interfaces #176

[FLINK-27279] Extract common status interfaces #176

Aitozi commented Apr 20, 2022 •

edited

Aitozi commented Apr 20, 2022

gyfora commented Apr 21, 2022 •

edited

Aitozi commented Apr 21, 2022

Aitozi commented Apr 22, 2022

gyfora commented Apr 22, 2022

gyfora Apr 22, 2022

gyfora Apr 22, 2022

Aitozi Apr 23, 2022

gyfora Apr 22, 2022

Aitozi Apr 23, 2022

gyfora Apr 23, 2022

gyfora left a comment

gyfora Apr 23, 2022

Aitozi Apr 23, 2022

gyfora Apr 23, 2022

Aitozi Apr 23, 2022

gyfora Apr 23, 2022

gyfora Apr 23, 2022

Aitozi Apr 23, 2022

Aitozi Apr 23, 2022

gyfora Apr 24, 2022

Aitozi Apr 24, 2022 •

edited

[FLINK-27279] Extract common status interfaces #176

[FLINK-27279] Extract common status interfaces #176

Conversation

Aitozi commented Apr 20, 2022 • edited

Aitozi commented Apr 20, 2022

gyfora commented Apr 21, 2022 • edited

Aitozi commented Apr 21, 2022

Aitozi commented Apr 22, 2022

gyfora commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aitozi Apr 24, 2022 • edited

Choose a reason for hiding this comment

Aitozi commented Apr 20, 2022 •

edited

gyfora commented Apr 21, 2022 •

edited

Aitozi Apr 24, 2022 •

edited