-
Notifications
You must be signed in to change notification settings - Fork 407
Add docs for Deploying with helm charts #1756 #1862
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
Conversation
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.
Thank you, @dreger1997 , the documentation is excellent. I left some comments.
| | [Kubernetes](https://kubernetes.io) | v1.19+ | v1.25+ | | ||
| | [Helm](https://helm.sh) | v3.8.0+ | v3.18.6+ | | ||
| | [ZooKeeper](https://zookeeper.apache.org) | v3.6+ | v3.8+ | | ||
| | [Apache Fluss](https://fluss.apache.org/docs/) (Container Image) | v0.8-SNAPSHOT | v0.8-SNAPSHOT | |
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.
Use variable $FLUSS_VERSION$ instead of v0.8-SNAPSHOT
| - Helm | ||
| - A running ZooKeeper ensemble (required for coordination) | ||
| - Sufficient cluster resources for the deployment | ||
| - **For Local Development**: Minikube and Docker (see [Local Development with Minikube](#local-development-with-minikube)) |
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.
This link #local-development-with-minikube cannot Jump to the correct dist.
| eval $(minikube docker-env) | ||
| ``` | ||
|
|
||
| To build custom images please refer to [Custom Image Configuration](#custom-image-configuration) |
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.
ditto. This link #custom-image-configuration cannot Jump to the correct dist.
| |-----------|-------------|---------| | ||
| | `image.registry` | Container image registry | `""` | | ||
| | `image.repository` | Container image repository | `fluss` | | ||
| | `image.tag` | Container image tag | `0.8-SNAPSHOT` | |
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.
ditto. Use variable $FLUSS_VERSION$ instead of v0.8-SNAPSHOT
| # Get detailed pod information | ||
| kubectl get pods -o wide -l app.kubernetes.io/name=fluss | ||
| ``` | ||
|
|
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.
Can we add a Interacting with Fluss part as other deploying docs do?
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.
I will do add a section with Flink
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.
Hi @dreger1997. Maybe I can merge it first. Then, If you have time, you can continue developing the Flink integration sub-module; otherwise, other developers can take over? What do you think?
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.
LGTM +1
|
|
||
| - Kubernetes | ||
| - Helm | ||
| - A running ZooKeeper ensemble (required for coordination) |
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.
nit: is this a prerequisite? step 1 of the installation guide explains how to set up zookeeper in case the user hasn't a zookeeper ensemble running yet.
| - Kubernetes | ||
| - Helm | ||
| - A running ZooKeeper ensemble (required for coordination) | ||
| - Sufficient cluster resources for the deployment |
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.
nit: when we say that a users needs "sufficient cluster resources," we should quantify what we consider "sufficient". if we cannot quantify it, this bullet should be removed.
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 @michaelkoepf , I agree with you. I have removed "ZooKeeper" and "Sufficient cluster resources" from the "Prerequisites". But added a Note under it to explain the ZooKeeper dependency.
| #### Install from Local Chart | ||
|
|
||
| ```bash | ||
| helm install fluss ./docker/helm |
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.
helm charts have been moved (#1917). if this PR does go into 0.9 (and not 0.8), we could update the path already.
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 @dreger1997 for the great documentation and I think it's already in a good shape. I appended a commit to resolve some minor issues and want to merge it first. If there is any other improvements, we can continue to polish it in separate PRs.
|
|
||
| ```bash | ||
| # TODO: Add chart to Helm repo. | ||
| helm repo add fluss https://charts.fluss.apache.org |
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.
We will publish the helm packages to https://downloads.apache.org/incubator/fluss/helm-chart
| 2. Build the Docker image: | ||
| ```bash | ||
| # Copy build artifacts | ||
| cp -r build-target/* docker/build-target/ |
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.
should be cp -r build-target/* docker/fluss/build-target, the fluss dockerfile has been moved to docker/fluss.
(cherry picked from commit 3046832)
Purpose
Linked issue: Add docs for Deploying with helm charts #1756
Adding a guide to deploy Fluss via Helm charts and how to interact with it.