[FLINK-26787] Initial implementation of FlinkSessionJobController and… #112

Aitozi · 2022-03-26T04:24:48Z

… reconciler

This PR is the initial implementation of FlinkSessionJobController and FlinkSessionJobReconciler. It handles the create and delete events only. Other spec change will introduce in the follow up ticket.

Aitozi · 2022-03-26T04:39:49Z

cc @gyfora @wangyang0918

gyfora

Thank you @Aitozi I think this is an excellent starting point for this feature. I have added some minor comments regarding class naming/packaging etc.

...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkSessionJobController.java

...netes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/BaseObserver.java

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

...src/main/java/org/apache/flink/kubernetes/operator/reconciler/FlinkSessionJobReconciler.java

...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/FlinkService.java

...perator/src/main/java/org/apache/flink/kubernetes/operator/validation/InternalValidator.java

helm/flink-operator/templates/flink-operator.yaml

...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/FlinkService.java

helm/flink-operator/values.yaml

helm/flink-operator/templates/flink-operator.yaml

wangyang0918 · 2022-03-27T09:16:07Z

I am trying to get to this PR and will share the comments today.

wangyang0918

Thanks @Aitozi for starting the concrete session job work. It looks already good and I left some minor comments.

examples/basic-session-job.yaml

...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/artifact/JarResolver.java

...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkSessionJobController.java

...va/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java

...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/FlinkService.java

helm/flink-operator/values.yaml

Aitozi · 2022-03-29T15:13:28Z

I have addressed your comments, partly left to follow up tickets, Please take a look again @wangyang0918

gyfora · 2022-04-01T12:19:30Z

I think we should try to merge this PR early next week, @Aitozi please coordinate with @bgeng777 and @SteNicholas regarding the other outstanding PRs (#141 & #131) that will likely confict with your change a little

Aitozi · 2022-04-01T13:05:00Z

OK, I will push a next commit to fix the @wangyang0918's recent comments about the event source and will solve the conflicts at the same time and will ping your guys for the final review

...rc/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java

… reconciler

docs/content/docs/development/guide.md

gyfora

Aside from the earlier comment regarding the validator interface I added a few minor things :)

But I think it looks really good

...perator/src/main/java/org/apache/flink/kubernetes/operator/config/OperatorConfigOptions.java

...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkSessionJobController.java

helm/flink-kubernetes-operator/conf/flink-operator-config/log4j2.properties

helm/flink-kubernetes-operator/templates/flink-operator.yaml

...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkSessionJobController.java

Aitozi · 2022-04-02T14:51:31Z

From my perspective, I think it's ready for review again. Please take look cc @gyfora @wangyang0918

gyfora · 2022-04-03T06:15:51Z

...rc/main/java/org/apache/flink/kubernetes/operator/validation/DefaultSessionJobValidator.java

 import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;

 import java.util.Optional;

 /** Default validator implementation for {@link FlinkSessionJob}. */
-public class DefaultSessionJobValidator implements FlinkResourceValidator<FlinkSessionJob> {
+public class DefaultSessionJobValidator implements FlinkResourceValidator {


Now that we have both methods in the interface can we drop the DefaultSessionJobValidator and just move the logic to the DefaultDeploymentValidator?

I dropped the DefaultSessionJobValidator. Please see my last commit. The webhook have not changed accordingly. I want to create another PR for it.
Now I use validate(FlinkSessionJob sessionJob, Optional<FlinkDeployment> session) to validate the session job. It will check the relations of both. But for webhook, it may not have access to the secondary resource, Maybe we could only verify the target session job in the webhook and do the full validation in the operator. what do you think of this ?

gyfora

Added some cosmetic comments but otherwise I think this is good to go 👍

...operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java

...or/src/main/java/org/apache/flink/kubernetes/operator/validation/FlinkResourceValidator.java

gyfora · 2022-04-03T12:59:08Z

...s-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/AdmissionHandler.java

@@ -62,7 +62,7 @@
 public class AdmissionHandler extends SimpleChannelInboundHandler<HttpRequest> {
    private static final Logger LOG = LoggerFactory.getLogger(AdmissionHandler.class);
    private static final ObjectMapper objectMapper = new ObjectMapper();
-    protected static final String VALIDATE_REQUEST_PATH = "/validate";
+    protected static final String VALIDATE_REQUEST_PATH = "/validateDeployment";


I think you need to adjust the webhook template in the helm chart if you are changing this

It's affected by the IDE refactor, reverted.

Aitozi force-pushed the session-job-reconciler branch from 1735b09 to c917ac6 Compare March 26, 2022 04:27

Aitozi changed the title ~~[FLINK-26787] Initial implementation of FlinkSessionJobController add…~~ [FLINK-26787] Initial implementation of FlinkSessionJobController and… Mar 26, 2022

gyfora requested changes Mar 26, 2022

View reviewed changes

bgeng777 reviewed Mar 26, 2022

View reviewed changes

...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/FlinkService.java Show resolved Hide resolved

gyfora reviewed Mar 26, 2022

View reviewed changes

helm/flink-operator/values.yaml Outdated Show resolved Hide resolved

helm/flink-operator/templates/flink-operator.yaml Outdated Show resolved Hide resolved

wangyang0918 reviewed Mar 27, 2022

View reviewed changes

Aitozi force-pushed the session-job-reconciler branch 2 times, most recently from 103b234 to aded648 Compare March 29, 2022 15:11

gyfora reviewed Apr 1, 2022

View reviewed changes

...rc/main/java/org/apache/flink/kubernetes/operator/validation/DefaultDeploymentValidator.java Outdated Show resolved Hide resolved

Aitozi force-pushed the session-job-reconciler branch from aded648 to d784f52 Compare April 2, 2022 13:26

[FLINK-26787] Initial implementation of FlinkSessionJobController and…

7a1b915

… reconciler

Aitozi force-pushed the session-job-reconciler branch from d784f52 to 7a1b915 Compare April 2, 2022 13:44

gyfora reviewed Apr 2, 2022

View reviewed changes

docs/content/docs/development/guide.md Show resolved Hide resolved

revert validator generic and fix compile error

f3b3946

gyfora requested changes Apr 2, 2022

View reviewed changes

Aitozi commented Apr 2, 2022

View reviewed changes

...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkSessionJobController.java Show resolved Hide resolved

fix comments

570fdde

gyfora requested changes Apr 3, 2022

View reviewed changes

drop the FlinkSessionJobValidator and move to DefaultValidator

fcf8bb0

Aitozi force-pushed the session-job-reconciler branch from a48bfb7 to fcf8bb0 Compare April 3, 2022 08:03

gyfora approved these changes Apr 3, 2022

View reviewed changes

gyfora reviewed Apr 3, 2022

View reviewed changes

fix comments

b91df2c

Aitozi force-pushed the session-job-reconciler branch from c591e5d to b91df2c Compare April 3, 2022 14:02

gyfora merged commit e4a6faa into apache:main Apr 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-26787] Initial implementation of FlinkSessionJobController and… #112

[FLINK-26787] Initial implementation of FlinkSessionJobController and… #112

Aitozi commented Mar 26, 2022

Aitozi commented Mar 26, 2022

gyfora left a comment

wangyang0918 commented Mar 27, 2022

wangyang0918 left a comment

Aitozi commented Mar 29, 2022

gyfora commented Apr 1, 2022

Aitozi commented Apr 1, 2022 •

edited

gyfora left a comment

Aitozi commented Apr 2, 2022

gyfora Apr 3, 2022

Aitozi Apr 3, 2022 •

edited

gyfora left a comment

gyfora Apr 3, 2022

Aitozi Apr 3, 2022

[FLINK-26787] Initial implementation of FlinkSessionJobController and… #112

[FLINK-26787] Initial implementation of FlinkSessionJobController and… #112

Conversation

Aitozi commented Mar 26, 2022

Aitozi commented Mar 26, 2022

gyfora left a comment

Choose a reason for hiding this comment

wangyang0918 commented Mar 27, 2022

wangyang0918 left a comment

Choose a reason for hiding this comment

Aitozi commented Mar 29, 2022

gyfora commented Apr 1, 2022

Aitozi commented Apr 1, 2022 • edited

gyfora left a comment

Choose a reason for hiding this comment

Aitozi commented Apr 2, 2022

gyfora Apr 3, 2022

Choose a reason for hiding this comment

Aitozi Apr 3, 2022 • edited

Choose a reason for hiding this comment

gyfora left a comment

Choose a reason for hiding this comment

gyfora Apr 3, 2022

Choose a reason for hiding this comment

Aitozi Apr 3, 2022

Choose a reason for hiding this comment

Aitozi commented Apr 1, 2022 •

edited

Aitozi Apr 3, 2022 •

edited