Skip to content
This repository was archived by the owner on May 8, 2025. It is now read-only.

Use StatefulSet instead of Deployment #354

Merged
merged 13 commits into from
Dec 8, 2020
Merged

Use StatefulSet instead of Deployment #354

merged 13 commits into from
Dec 8, 2020

Conversation

shashken
Copy link
Contributor

@shashken shashken commented Oct 20, 2020 •
edited
Loading

@functicons .
Closes #353

@shashken shashken mentioned this pull request Oct 20, 2020
Closed
@functicons
Copy link
Contributor

Thanks for the PR! I need some time to digest this change. Sorry for the delayed response.

@shashken
Copy link
Contributor Author

All good @functicons tell me if there is anything you want to discuss.

@functicons functicons self-requested a review October 22, 2020 16:02
@functicons
Copy link
Contributor

Sorry for the delayed response, I was super busy this week. I left some comments in #353, let's first discuss it there. Thanks!

functicons
functicons suggested changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@functicons functicons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add the new fields in the CRD doc and add a how to section for your use case in the user guide?

@shashken
Copy link
Contributor Author

@functicons Changed, sorry for the delay.
There are some function names that I havn't changed like 'reconcileDeployment' do you think we should change those names as well?

@functicons
Copy link
Contributor

Yes, I think it is better to update all of them.

@shashken shashken requested a review from functicons November 29, 2020 08:31
@shashken
Copy link
Contributor Author

Yes, I think it is better to update all of them.

Done

functicons
functicons suggested changes Dec 1, 2020
View reviewed changes
@shashken shashken requested a review from functicons December 1, 2020 12:54
@functicons
Copy link
Contributor

/gcbrun

@shashken
Copy link
Contributor Author

shashken commented Dec 3, 2020

@functicons I missed the review about CRD and doc, added them now. let me know if there is something else you want me to do before merging this.

functicons
functicons suggested changes Dec 4, 2020
View reviewed changes
@shashken shashken requested a review from functicons December 6, 2020 08:35
@thanidev
Copy link

thanidev commented Dec 7, 2020 •
edited
Loading

@shashken Can you please also add example sample Flink jobs and deployment involving RocksDB as state backend an SSD to config samples? This will be helpful!.

@functicons
Copy link
Contributor

Could you sync your repo and resolve the conflicts? @shashken

shashken and others added 2 commits December 8, 2020 18:25
@shashken
Merge pull request #1 from GoogleCloudPlatform/master
faba237 
changes
@shashken
Merge remote-tracking branch 'origin/master' into feature/useStatefulSet
fab1e20 
# Conflicts:
#	controllers/flinkcluster_converter.go
#	controllers/flinkcluster_observer.go
#	controllers/flinkcluster_reconciler.go
#	controllers/flinkcluster_util_test.go
@shashken
Copy link
Contributor Author

shashken commented Dec 8, 2020

@functicons resolved conflicts, please verify as the last PR merged today was a big one.
@thanidev Good idea, I will happily create a different PR for that, but as this one takes a bit too long I want to merge this first and then I'll create a new small PR with the example
If there aren't any problems lets merge this one, long lived PRs are not so healthy

@functicons
Copy link
Contributor

/gcbrun

@functicons
Copy link
Contributor

@shashken I have tested the PR, it worked perfectly, thanks!

@functicons functicons merged commit c817ad2 into GoogleCloudPlatform:master Dec 8, 2020
@burkaygur
Copy link

Looks like the current examples are not compatible with this PR. I am getting the following error:

2020-12-09T23:06:58.272Z	ERROR	controller-runtime.controller	Reconciler error	{"controller": "flinkcluster", "request": "default/flinksessioncluster-sample", "error": "FlinkCluster.flinkoperator.k8s.io \"flinksessioncluster-sample\" is invalid: [status.components.jobManagerStatefulSet: Required value, status.components.taskManagerStatefulSet: Required value]"}

@shashken shashken mentioned this pull request Dec 10, 2020
Open
shashken added a commit to shashken/flink-on-k8s-operator that referenced this pull request Dec 16, 2020
@shashken
Merge pull request #2 from GoogleCloudPlatform/master
324099e 
Use StatefulSet instead of Deployment (GoogleCloudPlatform#354)
@thanidev
Copy link

thanidev commented Dec 22, 2020 •
edited
Loading

@shashken @functicons Can you add examples - Sample Stateful Flink jobs and deployment involving RocksDB as state backend and SSD to config samples?

More examples are better for adoption of operator

@ghost
Copy link

ghost commented Jan 1, 2021

@shashken @functicons Would the latest operator image v1beta1-8 on gcr.io support the changes made to use StatefulSets? Looks like there are still references to Deployment when using the sample configs for the job and session clusters.

@shashken
Copy link
Contributor Author

shashken commented Jan 7, 2021

@gshen92 I dont know how or when those images are built, you can build the docker image from the master branch and upload to your container registry for now to see that it works :)
make operator-image push-operator-image IMG=<your_docker_registry>/flink-google-operator:0.0.<your_version>
And you can install the operator with the helm chart from the master branch as well.
@functicons Can probably help if you need an official image release

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@functicons functicons functicons approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use StatefulSet instead of Deployment for TMs and JM
4 participants
@shashken @functicons @thanidev @burkaygur