-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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-20359 Added Owner Reference to Job Manager in native kubernetes #14591
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 7100ec2 (Fri May 28 08:15:49 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
This is replacement for #14512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blublinsky The changes now look really good to me. Could you please regenerate the config docs? Currently, the azure pipeline failed because of the doc tests.
mvn package -Dgenerate-config-docs -pl flink-docs -am -nsu -DskipTests
Please do not delete the remote repository and just force push again after updates so that we could keep all the changes in one PR.
I have run a Flink application on minikube with setting the owner reference of JobManager deployment to a new created service(e.g. my-test-svc). After the service is deleted, all the application K8s related resources, including JobManager deployment, could be deleted by GC. This PR works well. |
ping @blublinsky Could you please address the comments before merging? |
Sorry about missing this. Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for merging.
cc @xintongsong Could you please have a final check and help with merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @blublinsky for preparing the PR, and @wangyang0918 for the review.
The feature looks really good to me. I have only minor comments about the documentation. Please take a look.
...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java
Outdated
Show resolved
Hide resolved
Sorry @xintongsong, are we done now? |
Sorry, closed the PR by mistake. @blublinsky, you have not addressed my comment.
The above should be explained in the configuration option description. |
"Why is the owner reference only set for JobManager. Are other resources guaranteed to be cleaned up? We might want to make it more explicit that "delete the actual cluster" means all resources are cleaned up." How exactly to use the configuration option. I would suggest to add an example (with multiple owner references). |
By explain in the configuration option description, I don't mean to explain as comments in this GitHub Pull Request. In the file So what I meant is that we should explain the two questions in this description. |
@xintongsong , done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @blublinsky, taking over from here.
402056b
to
7100ec2
Compare
…ernetes This closes #14591
Thanks @xintongsong for refining the description and merging. |
…ernetes This closes apache#14591
What is the purpose of the change
Flink implementation is often a part of the larger application. As a result a synchronized management - clean up of Flink resources, when a main application is deleted is important. In Kubernetes, a common approach for such clean up is usage of the owner's reference (https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/)
This PR allows adding owner reference support to Flink Job manager
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Running job manager factory unit test
Does this pull request potentially affect one of the following parts:
Documentation