-
Notifications
You must be signed in to change notification settings - Fork 19
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
update chart #128
update chart #128
Conversation
Thanks for the commit. LGTM but I'm not experienced with helm configs. I'll merge by tomorrow if others have no comments |
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.
Good work! I've left some comments, I'll try it later today, thanks
kubernetes/bkvm/values.yaml
Outdated
imagePullPolicy: Always | ||
|
||
jdbcUrl: jdbc:herddb:local:temporary | ||
metadataServiceUri: zk://pulsar-zookeeper:2181/ledgers |
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.
Could you remove the pulsar reference?
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.
Do you want it empty or this string "zk://zookeeper:2181/ledgers" instead ?
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 think that most of the users currently are using Pulsar
I would keep pulsar-zookeeper in order to make it easier
Not a big deal as far as we keep this configurable
maybe you can keep a comment to explain how to use it with the Apache Pulsar helm chart
labels: | ||
{{ toYaml .Values.server.labels | nindent 4 }} | ||
spec: | ||
replicas: 1 |
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 should be configurable
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 don't think that BKVM supports multiple instances at the moment (because there is no handling of distributed sessions).
I think that we should keep 1 here
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 didn't test it with several replicas but one replica works with Pulsar
good work @enzo-dechaene ! |
Following this checklist to help us incorporate your
contribution quickly and easily:
mvn clean verify
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.