Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13568 )
Change subject: KUDU-2834: Build Kudu Kubernetes cluster using Helm chart ...................................................................... Patch Set 4: (10 comments) Thanks for the contribution! I have a few comments based on initial review and will do more review later. Is there some way we can ensure this works as it changes and evolves? Some sort of unit/integration testing that can be run? http://gerrit.cloudera.org:8080/#/c/13568/4/docker/kudu-entrypoint.sh File docker/kudu-entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/13568/4/docker/kudu-entrypoint.sh@80 PS4, Line 80: sleep 2; Curious about this change. Was 1 second not enough in some cases? Are we sure 2 is enough? Maybe there is a better way to do this. http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/README.adoc File kubernetes/README.adoc: http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/README.adoc@67 PS4, Line 67: nit: white space http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/README.adoc File kubernetes/helm/README.adoc: PS4: Does this readme need to be separate? Could it be merged with the readme one level up and describe when to use helm vs kubectl? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/kudu/Chart.yaml File kubernetes/helm/kudu/Chart.yaml: http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/kudu/Chart.yaml@20 PS4, Line 20: appVersion: "1.0" Should this match the Kudu version? Can it be sourced from version.txt? I am referencing this: https://helm.sh/docs/developing_charts/#the-chart-yaml-file http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/kudu/Chart.yaml@21 PS4, Line 21: description: Official Kudu Helm Chart Should this be a description of Kudu, not the "chart"? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/kudu/Chart.yaml@23 PS4, Line 23: version: latest How should this version be set going forward? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/helm/kudu/Chart.yaml@25 PS4, Line 25: home: https://kudu.apache.org/ Can you also add the `sources` field? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/kudu-services.yaml File kubernetes/kudu-services.yaml: http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/kudu-services.yaml@18 PS4, Line 18: # More about kubernetes statefulset https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/ Should these docs live in the readme? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/kudu-services.yaml@29 PS4, Line 29: kind: Service Can you add a comment about this service similar to the others below? http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/kudu-statefulset.yaml File kubernetes/kudu-statefulset.yaml: http://gerrit.cloudera.org:8080/#/c/13568/4/kubernetes/kudu-statefulset.yaml@20 PS4, Line 20: # create services for kudu kubernetes statefulset cluster : kubectl create -f kudu-services.yaml Should these docs live in the readme now? -- To view, visit http://gerrit.cloudera.org:8080/13568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f839b690322fc349a566ee643d39f039f72ffa6 Gerrit-Change-Number: 13568 Gerrit-PatchSet: 4 Gerrit-Owner: Sandish Kumar HN <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Jun 2019 13:43:25 +0000 Gerrit-HasComments: Yes
