-
Notifications
You must be signed in to change notification settings - Fork 212
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
Tiered Storage config #205
Conversation
Hey @michaeljmarshall! @lhotari & @cdbartholomew mentioned that we should ping you when we made this contribution 👍 Let us know if there is anything missing or what we can do to move this forward! |
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.
@pellicano - thanks for your contribution! I left some minor comments that we should address and then it'll be ready to merge.
Remove <= 2.6.0 configs. Add missing GCS secret volumeMount. Update GCS example name.
Thanks for the comments @michaeljmarshall! |
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.
LGTM. Thanks for addressing my feedback, @pellicano.
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 @pellicano . Thanks for contributing!
@sijie @codelipenghui @eolivelli @addisonj This PR would be a valuable addition to Apache Pulsar Helm chart. Please review. |
@MarvinCai @tuteng can you review this change? |
Moved storageOffload under broker section. Fixed some typos. Added AWS S3 IRSA annotation comment.
Changes applied as suggested by @sijie. Thank you. |
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.
@tuteng raises an important security concern, I'm switching my review to request changes until we move the keys to secrets.
Moved AWS and Azure credentials into K8S secrets using same StreamNative Helm Chart approach.
Addressed security concern related to AWS and Azure credentials. |
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.
LGTM.
@sijie - PTAL |
@sijie if you may take a look, I appreciate. Thank you. |
Any news here, @michaeljmarshall? |
@sijie Can you assign over to someone else to take a look? |
@pellicano @ckdarby - are you able to rebase and then take a look at the failing status checks? Once that happens, I think we can merge this. Thanks! |
If needed I can open a new PR with this rebased. |
# Conflicts: # charts/pulsar/Chart.yaml
@pellicano - looks like some of the tests failed with this error:
I assume this fails because it's comparing an unset value to a string value. |
Fixes #72
Motivation
Comply with Tiered Storage configuration.
https://pulsar.apache.org/docs/en/concepts-tiered-storage/
https://pulsar.apache.org/docs/en/cookbooks-tiered-storage/
Modifications
Add a modified Helm Chart version from https://github.com/kafkaesque-io/pulsar-helm-chart.
Other reference Helm Chart: https://github.com/streamnative/charts
Verifying this change