Skip to content

feat: Base Helm chart#45

Open
EladLeev wants to merge 3 commits intounitycatalog:mainfrom
EladLeev:main
Open

feat: Base Helm chart#45
EladLeev wants to merge 3 commits intounitycatalog:mainfrom
EladLeev:main

Conversation

@EladLeev
Copy link

Create a base Helm chart for Unity Catalog based on #18.
Let me know if the repository or image name should be changed.

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added or modified a feature, documentation in docs is updated

Description of changes

@EladLeev EladLeev changed the title create base Helm chart feat: Base Helm chart Jun 15, 2024
@tdas
Copy link
Contributor

tdas commented Jun 17, 2024

Forgive me lack of knowledge, but what is Helm?

@dennyglee
Copy link
Contributor

If I'm not mistaken, this is to deploy UC on a K8 cluster? @EladLeev - by any chance do you have any verification scripts outside the the connection test?

@EladLeev
Copy link
Author

Yes, it's for deploying UC on top of K8s.
To test that I built the Docker image locally based on #18 (had to make some changes to make it work tbh), and deployed it using this Helm chart on minikube.
You can then port-forward to the service, and use the CLI normally:

kubectl port-forward svc/unitycatalog 8081:8080

Just to note, I assumed that the org and image name are unitycatalog here, if that's not the case, it's better to change the defaults

Thanks! 🙏

@dennyglee
Copy link
Contributor

Thanks @EladLeev - will review as soon as I can - much appreciated!

@dnskr
Copy link

dnskr commented Aug 4, 2024

Unity Catalog Helm chart is a great initiative!

In my opinion, the Helm chart implementation looks very basic in the current state. After a brief review I found some misconfigurations that are concerning even for the initial version of the chart:

  • Missing appVersion. It is an optional field (usually specified), however it is used in the chart templates.
  • Port 8081 is used instead of 8080.
  • Property replicaCount is configurable. However, if I understand it correctly, current UC implementation allows to use in-memory or file h2 database only, so database cannot be shared between UC instances. It means that users might get different state from different UC instances.
  • H2 file database cannot persist between pod restarts, i.e. UC starts fresh after pod restart.
  • The chart supports Kubernetes 1.12+. Usually only latest three versions of Kubernetes are supported (1.30, 1.29, 1.28 at the moment) to avoid complex templating of different APIs. For instance, Ingress template is overloaded by different checks and cases for old Kubernetes versions.
  • UC configuration is not supported in the chart, i.e. users cannot specify UC configs in any way.

dennyglee pushed a commit that referenced this pull request Sep 2, 2024
* delete table feature

* handle onSuccess callback, navigate, display notification

* PR feedback

* More pr feedback

* change button type

* tableFullName prop

* memoize
tdas pushed a commit that referenced this pull request Sep 5, 2024
* delete table feature

* handle onSuccess callback, navigate, display notification

* PR feedback

* More pr feedback

* change button type

* tableFullName prop

* memoize
rtyler pushed a commit to rtyler/unitycatalog that referenced this pull request Sep 5, 2024
* delete table feature

* handle onSuccess callback, navigate, display notification

* PR feedback

* More pr feedback

* change button type

* tableFullName prop

* memoize
@missedone
Copy link

@EladLeev , great initiative as i'm looking for the helm chart to quickly spin up in the k8s to test out UC.

@missedone
Copy link

H2 file database cannot persist between pod restarts, i.e. UC starts fresh after pod restart.

To my knowledge, H2 is not for production environments; it is mostly used in dev and testing environments. Do you know if UC will support other databases like PostgreSQL, MySQL, etc.?

@dnskr
Copy link

dnskr commented Sep 8, 2024

H2 file database cannot persist between pod restarts, i.e. UC starts fresh after pod restart.

To my knowledge, H2 is not for production environments; it is mostly used in dev and testing environments. Do you know if UC will support other databases like PostgreSQL, MySQL, etc.?

Looks like Unity Catalog 0.2 (in development at the moment) supports other databases as well through providing custom Hibernate configuration and adding jars files with JDBC drivers to UC classpath.

@ion-elgreco
Copy link

Why isn't this merged yet?

@dennyglee
Copy link
Contributor

Why isn't this merged yet?

Thanks for the reminder @ion-elgreco - I think a bunch of folks are currently out for the holidays. Would you be up for reviewing this?

@dnskr
Copy link

dnskr commented Dec 21, 2024

@ion-elgreco Probably the author doesn’t have time or not interested in continuing the development. For instance, he didn't address the issues from my comment #45 (comment).

I started working on alternative PR some time ago, but unfortunately I also had to change my focus. I'm going to get back to it during holidays and check if latest UC release made operational aspects more clear and convenient to support it in Helm chart.

@ion-elgreco
Copy link

@dennyglee @dnskr I might be able to allocate some time to this after January next year for either reviewing or helping to push this over the finish line

@EladLeev
Copy link
Author

Same here - I can revisit it on January and push an update. There are some missing features and toggles from this PR.

@dennyglee
Copy link
Contributor

Thanks in advance @EladLeev and @ion-elgreco . Let's definitely plan on restarting the push in January. IF it makes sense, perhaps for one of our community sync's we can dive into this as well?

@tnightengale
Copy link
Collaborator

Following

@dennyglee
Copy link
Contributor

Hey @EladLeev - would it be helpful if we used one of the next community meetups to specifically discuss the design/issues related to this? Feel free to respond here and/or slack me via the Unity Catalog Slack to discuss at your convenience. Much appreciated!

@hongbo-miao
Copy link

I guess this will be replaced by #895 (?)
Either way, cannot wait for it to be ready! ☺️

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants