helm chart fix/update by oliverdding · Pull Request #11427 · apache/druid

oliverdding · 2021-07-09T15:34:21Z

Key changed/added classes in this PR

helm chart

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

* Remove licence comment at the beginning of template files * Remove sub-charts * Remove some configs that has default value * Move `druid.segmentCache.locations` default value to the mounted path * Add hdfs's mount point in historical node and middlemanager node

helm/druid/README.md

helm/druid/values.yaml

helm/druid/Chart.yaml

Co-authored-by: Benedict Jin <asdf2014@apache.org>

asdf2014

LGTM, thank you for your contribution

suneet-s · 2021-08-09T14:49:17Z

@asdf2014 is there anything left to merge this change?

asdf2014 · 2021-08-09T15:08:01Z

@suneet-s No, all good

suneet-s · 2021-08-09T16:14:25Z

@oliverdding thanks for the contribution. I don't know a lot about helm charts. How can we validate that the helm chart continues to work as new code changes come in? Are there any integration tests that will prove this works with every release of Apache Druid?

oliverdding · 2021-08-10T06:41:17Z

@oliverdding thanks for the contribution. I don't know a lot about helm charts. How can we validate that the helm chart continues to work as new code changes come in? Are there any integration tests that will prove this works with every release of Apache Druid?

As far as I known, no... Helm chart maintainer have to konw the great picture of Druid, and helm chart is like a script for deploying, only who had read the whole chart may tell what should be changeh in the next release...

abhishekagarwal87 · 2021-08-11T10:07:39Z

Thank you for your contribution @oliverdding. I had some questions and suggestions

what zk ensemble, the druid services will be initialized with after your changes? We would still need to pass druid_zk_service_host somehow?
Instead of removing mysql and postgres entirely, I will suggest keeping them with enabled as false in default values.yaml. You can have two more values.yaml files inside mysql and postgres folder that a user can choose to override default values.yaml with.

abhishekagarwal87 · 2021-08-11T10:08:53Z

helm/druid/values.yaml

-  druid_metadata_storage_connector_connectURI: jdbc:postgresql://postgres:5432/druid
-  druid_metadata_storage_connector_user: druid
-  druid_metadata_storage_connector_password: druid
+  druid_extensions_loadList: '["druid-datasketches", "druid-lookups-cached-global", "postgresql-metadata-storage"]'


what is the reason for removing druid-histogram from here?

Because the doc says that it's deprecated and should use druid-datasketches instead.

abhishekagarwal87 · 2021-08-11T10:10:14Z

helm/druid/values.yaml


  ## Druid Common Configurations. ref: https://druid.apache.org/docs/latest/configuration/index.html#common-configurations
-  druid_extensions_loadList: '["druid-histogram", "druid-datasketches", "druid-lookups-cached-global", "postgresql-metadata-storage"]'
-  druid_metadata_storage_type: postgresql


as suggested, this whole section can go into postgres/values.yaml so existing users can still use postgres without changing the chart etc.

abhishekagarwal87 · 2021-08-11T10:15:23Z

helm/druid/values.yaml

+  # 2. add `"druid-hdfs-storage"` to `druid_extensions_loadList`
+  # 3. supply configmap's name which contains hdfs-site.xml and core-site.xml
+  enabled: false
+  configMapName: ''


can you add an example in hdfs/values.yaml? It will also be a bonus if you can add configmap template for HDFS in templates. That configmap will be deployed only if .values.hdfs.enabled is set to true.

The configmap is looks like:

apiVersion: v1 kind: ConfigMap data: core-site.xml: | <balabala something here> hdfs-site.xml: | <balabala something here>

In fact, the configmap would be generate by every deployment of hdfs, which ought to contain the configurations that hdfs client needs when accessing hdfs. I did not supply a template file for that configmap, since when deploying druid with hdfs as deep storage, the configmap would have been in place.

oliverdding · 2021-08-11T11:18:56Z

Thank you for your contribution @oliverdding. I had some questions and suggestions

what zk ensemble, the druid services will be initialized with after your changes? We would still need to pass druid_zk_service_host somehow?

Instead of removing mysql and postgres entirely, I will suggest keeping them with enabled as false in default values.yaml. You can have two more values.yaml files inside mysql and postgres folder that a user can choose to override default values.yaml with.

Yes, you need to pass druid_zk_service_host if you want to use druid under zookeeper, but I consider that free from zookeeper is the future run without zookeeper, so we should leave this blank in the base chart.
I did this considering that:
- That's not K.I.S.S. User may want to deploy druid with mysql, may want to deploy without zookeeper... Though postgresql and zookeeper are our dependencies, we should not package them with our application.
- User should deploy their own pod to fill in their own business. For example, my company has postgresql deployed as base componment, so I just want the druid to use that one instead of taking it's own without deployed.

oliverdding · 2021-08-11T11:46:59Z

@abhishekagarwal87 I don't think packing other based componments with druid is a good idea. What's wanted would be deployed by users, what's not would bother users.

abhishekagarwal87 · 2021-08-11T12:08:34Z

Thank you for your contribution @oliverdding. I had some questions and suggestions

what zk ensemble, the druid services will be initialized with after your changes? We would still need to pass druid_zk_service_host somehow?

Instead of removing mysql and postgres entirely, I will suggest keeping them with enabled as false in default values.yaml. You can have two more values.yaml files inside mysql and postgres folder that a user can choose to override default values.yaml with.

Yes, you need to pass druid_zk_service_host if you want to use druid under zookeeper, but I consider that free from zookeeper is the future run without zookeeper, so we should leave this blank in the base chart.

I did this considering that:

That's not K.I.S.S. User may want to deploy druid with mysql, may want to deploy without zookeeper... Though postgresql and zookeeper are our dependencies, we should not package them with our application.

User should deploy their own pod to fill in their own business. For example, my company has postgresql deployed as base componment, so I just want the druid to use that one instead of taking it's own without deployed.

Most of the deployments are still zk based so the helm chart should support that.
That can be achieved by tweaking the conditions. You can introduce new values such as install.postgres, install.zookeeper, and install.mysql that can be false by default. You can use these flags as conditions for deciding whether to download a dependency.
As suggested, please also add values.yaml that are specific to mysql and postgres that a user can pass in addition to the default values. The values.yaml inside mysql folder will look something like this

mysql:
  enabled: true
  mysqlRootPassword: druidroot
  mysqlUser: druid
  mysqlPassword: druid
  mysqlDatabase: druid
  configurationFiles:
    mysql_collate.cnf: |-
      [mysqld]
      character-set-server=utf8
      collation-server=utf8_unicode_ci

default root-directory values.yaml will have the following section

mysql:
  enabled: false

same for postgres. The changes in configmap.yaml can be reverted.

if a user wants to run with mysql enabled, he/she can run the helm install with values.yaml -f mysql/values.yaml

oliverdding · 2021-08-14T06:15:20Z

Thank you for your contribution @oliverdding. I had some questions and suggestions

what zk ensemble, the druid services will be initialized with after your changes? We would still need to pass druid_zk_service_host somehow?

Instead of removing mysql and postgres entirely, I will suggest keeping them with enabled as false in default values.yaml. You can have two more values.yaml files inside mysql and postgres folder that a user can choose to override default values.yaml with.

Yes, you need to pass druid_zk_service_host if you want to use druid under zookeeper, but I consider that free from zookeeper is the future run without zookeeper, so we should leave this blank in the base chart.

I did this considering that:

That's not K.I.S.S. User may want to deploy druid with mysql, may want to deploy without zookeeper... Though postgresql and zookeeper are our dependencies, we should not package them with our application.

User should deploy their own pod to fill in their own business. For example, my company has postgresql deployed as base componment, so I just want the druid to use that one instead of taking it's own without deployed.
1. Most of the deployments are still zk based so the helm chart should support that.

2. That can be achieved by tweaking the conditions. You can introduce new values such as `install.postgres`, `install.zookeeper`, and `install.mysql` that can be `false` by default. You can use these flags as conditions for deciding whether to download a dependency.

3. As suggested, please also add values.yaml that are specific to mysql and postgres that a user can pass in addition to the default values. The `values.yaml` inside `mysql` folder will look something like this
mysql:
  enabled: true
  mysqlRootPassword: druidroot
  mysqlUser: druid
  mysqlPassword: druid
  mysqlDatabase: druid
  configurationFiles:
    mysql_collate.cnf: |-
      [mysqld]
      character-set-server=utf8
      collation-server=utf8_unicode_ci
default root-directory values.yaml will have the following section
mysql:
  enabled: false
same for postgres. The changes in configmap.yaml can be reverted.

if a user wants to run with mysql enabled, he/she can run the helm install with values.yaml -f mysql/values.yaml

Sorry for replaying late. I'm convinced.

But I got a problem...I have deleted the forked repository when this MR is accepted...

abhishekagarwal87 · 2021-08-14T11:09:27Z

@oliverdding you can try to create a new fork and checkout this PR into a branch in that fork.

stale · 2022-04-28T08:09:14Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

github-actions · 2023-11-07T00:16:31Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

oliverdding and others added 2 commits July 9, 2021 23:31

Merge branch 'apache:master' into master

18c4100

asdf2014 added the Helm Chart https://github.com/apache/druid/tree/master/helm/druid label Jul 10, 2021

asdf2014 reviewed Jul 10, 2021

View reviewed changes

helm/druid/README.md Outdated Show resolved Hide resolved

helm/druid/values.yaml Outdated Show resolved Hide resolved

asdf2014 reviewed Jul 11, 2021

View reviewed changes

helm/druid/Chart.yaml Show resolved Hide resolved

oliverdding and others added 4 commits July 11, 2021 17:11

Update helm/druid/README.md

428ce6a

Co-authored-by: Benedict Jin <asdf2014@apache.org>

Update helm/druid/values.yaml

c656288

Co-authored-by: Benedict Jin <asdf2014@apache.org>

helm: Add license headers back

63f66cf

Merge branch 'apache:master' into master

8f17e30

asdf2014 approved these changes Jul 11, 2021

View reviewed changes

abhishekagarwal87 reviewed Aug 11, 2021

View reviewed changes

stale bot added the stale label Apr 28, 2022

github-actions bot closed this Nov 7, 2023

Conversation

oliverdding commented Jul 9, 2021

Key changed/added classes in this PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asdf2014 left a comment

Choose a reason for hiding this comment

Uh oh!

suneet-s commented Aug 9, 2021

Uh oh!

asdf2014 commented Aug 9, 2021

Uh oh!

suneet-s commented Aug 9, 2021

Uh oh!

oliverdding commented Aug 10, 2021

Uh oh!

abhishekagarwal87 commented Aug 11, 2021

Uh oh!

abhishekagarwal87 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

oliverdding Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

oliverdding Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

oliverdding commented Aug 11, 2021

Uh oh!

oliverdding commented Aug 11, 2021

Uh oh!

abhishekagarwal87 commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliverdding commented Aug 14, 2021

Uh oh!

abhishekagarwal87 commented Aug 14, 2021

Uh oh!

stale bot commented Apr 28, 2022

Uh oh!

github-actions bot commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abhishekagarwal87 commented Aug 11, 2021 •

edited

Loading