Improve the deploy_with_existing_vineyard_cluster doc for easy understanding
#2930
Conversation
|
🎊 PR Preview 8e39724 has been successfully built and deployed to https://alibaba-graphscope-build-pr-2930.surge.sh 
🤖 By surge-preview |
dashanji
force-pushed
the
add-pictures
branch
2 times, most recently
from
6f1c330
to
13485da
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -36,12 +50,17 @@ python3 -m pip install vineyard | |||
| ``` | |||
|
|
|||
| By default, the Vineyard cluster consists of three Vineyard instances and three etcd instances. | |||
| However, since we only have one node in the Kubernetes cluster, we need to specify the number of Vineyard instances and etcd instances using the `vineyard_replicas` and `vineyard_etcd_replicas` parameters. | |||
| However, since we only have one node in the Kubernetes cluster, we need to specify the number of Vineyard instances and etcd instances using the `vineyard_replicas` and `vineyard_etcd_replicas` parameters. DON'T set the number of Vineyard instances and etcd instances to be greater than the number of nodes in the Kubernetes cluster. | |||
|
|
|||
There was a problem hiding this comment.
etcd replica could have a reasonable default, and don't let the user care those details in the getting started tutorial.
There was a problem hiding this comment.
Could we raise error when the number of vineyard instances greater than the number of nodes?
There was a problem hiding this comment.
Could we raise error when the number of vineyard instances greater than the number of nodes?
It should be managed by the users rather than vineyardctl. Also, it won't block the creation of the vineyard cluster. if the number of vineyard instances is greater than the number of nodes, the other vineyard pods will be pending.
| # The default namespace is `vineyard-system` and the default deployment name is `vineyardd-sample`. Also, the command will create the namespace if it doesn't exist. | ||
| # For more details about the parameters, please refer to the doc of vineyardctl | ||
| # https://github.com/v6d-io/v6d/blob/main/k8s/cmd/README.md | ||
| # Notice, all character `-` in the parameter of vineyardctl should be replaced with `_` in the python API | ||
| vineyard.deploy.vineyardctl.deploy.vineyard_deployment( | ||
| vineyard_replicas=1, |
There was a problem hiding this comment.
Only leave the vineyard_replicas is okay, the others could use the default.
Also since you already use the name vineyard_deployment, why you prefix every parameters with vineyard_? for example, just vineyard_deployment(replicas=1, etcd_replicas=2) is meaningful enough for me.
There was a problem hiding this comment.
Only leave the
vineyard_replicasis okay, the others could use the default. Also since you already use the namevineyard_deployment, why you prefix every parameters withvineyard_? for example, justvineyard_deployment(replicas=1, etcd_replicas=2)is meaningful enough for me.
Thanks for the feedback, I will change the parameters in the next version.
|
|
||
| ```python | ||
| import vineyard | ||
| vineyard.deploy.vineyardctl.delete.vineyard_deployment() |
There was a problem hiding this comment.
delete doesn't require the deployment name?
There was a problem hiding this comment.
There is a default deployment name vineyardd-sample, as we create the vineyard deployment with the default name.
There was a problem hiding this comment.
emm, I think better let the delete be explicit.
* Add the pictures for easy understanding. * Add the details and fix some errors. Signed-off-by: Ye Cao <caoye.cao@alibaba-inc.com>
* Explain the lifecycle of a vineyard cluster. Signed-off-by: Ye Cao <caoye.cao@alibaba-inc.com>
|
LGTM. |
What do these changes do?
Improve the
deploy_with_existing_vineyard_clusterdoc.Related issue number
Fixes #2549