Add yorkie-analytics Helm chart with Starrocks and Kafka #1161
Conversation
WalkthroughThis pull request introduces a new Helm chart for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1161 +/- ##
=======================================
Coverage 38.56% 38.56%
=======================================
Files 170 170
Lines 25441 25441
=======================================
Hits 9812 9812
Misses 14808 14808
Partials 821 821 ☔ View full report in Codecov by Sentry. |
emplam27
force-pushed
the
helm-kafka-chart
branch
from
a52bf69 to
c179aab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml (1)
14-16: Remove Trailing Spaces
There appears to be trailing whitespace on line 15. Removing these extraneous spaces will help ensure YAML lint compliance.Proposed Diff:
- KAFKA_HOST={{ .Values.name }}-kafka.{{ .Values.namespace }}.svc.cluster.local:9092 + KAFKA_HOST={{ .Values.name }}-kafka.{{ .Values.namespace }}.svc.cluster.local:9092🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: trailing spaces
(trailing-spaces)
build/charts/yorkie-analytics/templates/kafka/job.yaml (1)
29-32: Correct Volumes Indentation
The YAML linter warns that the indentation under thevolumeskey is off (expected indent of 8 spaces for the list items undervolumes). Adjust the indentation of the volume list to improve YAML readability and prevent potential parsing issues.Proposed Diff:
- volumes: - - name: init-kafka-topics-script - configMap: - name: {{ .Values.name }}-init-kafka-topics-script + volumes: + - name: init-kafka-topics-script + configMap: + name: {{ .Values.name }}-init-kafka-topics-script🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 30-30: wrong indentation: expected 8 but found 6
(indentation)
build/charts/yorkie-analytics/values.yaml (1)
1-66: Optional: Enhance Documentation in Values File
While the configuration is clear, consider adding inline comments or descriptions for key sections (such asroutine-load,kube-starrocks, and Kafka’s specifications). This can help future maintainers understand the purpose of each configuration block more quickly.build/charts/yorkie-analytics/templates/starrocks/configmap.yaml (3)
11-28: SQL script for database and table creation.
Theinit-user-events-db.sqlsection creates the database and table with the intended schema. Please remove any trailing whitespace (as flagged by static analysis) to avoid formatting issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
29-41: Routine load SQL configuration is defined properly.
Theinit-routine-load.sqlsection correctly sets up the routine load with templated Kafka configuration values. Again, address any trailing space issues as reported by YAMLlint.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 37-37: trailing spaces
(trailing-spaces)
43-73: Shell script for StarRocks initialization is comprehensive.
Theinit-starrocks-database.shscript uses a series of sleep commands and status checks to ensure services are ready before executing SQL scripts. Consider addingset -eat the top of the script so that any command failure aborts the process—this can improve error handling in critical initialization steps.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
build/charts/yorkie-analytics/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
build/charts/yorkie-analytics/.helmignore(1 hunks)build/charts/yorkie-analytics/Chart.yaml(1 hunks)build/charts/yorkie-analytics/templates/NOTES.txt(1 hunks)build/charts/yorkie-analytics/templates/kafka/configmap.yaml(1 hunks)build/charts/yorkie-analytics/templates/kafka/job.yaml(1 hunks)build/charts/yorkie-analytics/templates/starrocks/configmap.yaml(1 hunks)build/charts/yorkie-analytics/templates/starrocks/job.yaml(1 hunks)build/charts/yorkie-analytics/values.yaml(1 hunks)build/charts/yorkie-cluster/templates/yorkie/deployment.yaml(1 hunks)build/docker/analytics/docker-compose.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- build/charts/yorkie-analytics/.helmignore
🧰 Additional context used
🪛 YAMLlint (1.35.1)
build/charts/yorkie-analytics/templates/starrocks/configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml
[error] 15-15: trailing spaces
(trailing-spaces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
build/charts/yorkie-analytics/templates/kafka/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 30-30: wrong indentation: expected 8 but found 6
(indentation)
build/charts/yorkie-analytics/templates/starrocks/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (18)
build/charts/yorkie-analytics/templates/NOTES.txt (1)
1-10: Clear Installation and Next Steps Message
The installation message and subsequent next steps instructions are clear and helpful. The usage of the Helm templating variable ({{ .Release.Name }}) is correct for dynamically displaying the release name.build/charts/yorkie-analytics/templates/kafka/configmap.yaml (1)
1-24: Well-Structured Conditional ConfigMap for Kafka Topics
The ConfigMap is correctly wrapped in a conditional block that checks if Kafka is enabled. The embedded shell script correctly constructs and uses the Kafka host and runs the topic initialization commands.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: trailing spaces
(trailing-spaces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
build/charts/yorkie-analytics/Chart.yaml (1)
1-35: Comprehensive Helm Chart Metadata
The Chart.yaml file is well populated with all the necessary metadata, maintainers’ details, sources, and dependency definitions. The use of conditions for enabling StarRocks and Kafka dependencies is clear and appropriate.build/charts/yorkie-analytics/templates/kafka/job.yaml (3)
1-35: Solid Definition of Kafka Initialization Job
The Job resource is well defined and conditionally deployed based on the Kafka enabled flag. The use of an init container to wait for the Kafka controller’s readiness, proper annotation for Helm hooks, and the defined backoff limit are all implemented effectively.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 30-30: wrong indentation: expected 8 but found 6
(indentation)
16-17: Verify Service Account Usage
The Job specifiesserviceAccountName: starrocks(line 16). Please confirm that using the StarRocks service account for a Kafka initialization job is intentional. If this was meant to use a Kafka-specific service account, then adjust accordingly.
1-1: Helm Templating Directive Note
The leading templating directive ({{- if ... }}) on line 1 can sometimes trigger static analysis syntax errors in plain YAML linters. This is a known false positive due to Helm’s templating syntax and can be safely ignored.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
build/charts/yorkie-analytics/values.yaml (1)
1-66: Comprehensive Configuration for Yorkie Analytics Deployment
The values.yaml file neatly organizes configuration for both theyorkie-analyticsapplication and its dependent components (StarRocks and Kafka). The use of YAML anchors for the StarRocks image repository and tag is an excellent way to avoid redundancy. All critical parameters—including enabled flags, image repositories/tags, and resource settings—are provided and clearly structured.build/charts/yorkie-analytics/templates/starrocks/job.yaml (4)
1-12: Helm templating condition & metadata annotations check.
The conditional block using{{- if index .Values "yorkie-analytics" "starrocks" "enabled" }}ensures that the Job resource is only created when StarRocks is enabled. The metadata (name, namespace, labels, and Helm hook annotations) are correctly set. Note that YAML lint errors on the leading dash in templating (as reported by YAMLlint) are common false positives in such files.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
18-29: Init containers for service readiness are well configured.
The use of multiple init containers to wait for the StarRocks front-end (wait-for-starrocks-fe) and back-end (wait-for-starrocks-be), plus conditionally waiting on Kafka (viawait-for-kafka), helps ensure that all dependent services are ready before running the database initialization script.
31-37: Main database initialization container & volume mounts.
The primary container (init-starrocks-database) runs the initialization script from/etc/config/init-starrocks-database.shwith its associated volume mount. This separation of concerns is clear and maintainable.
38-41: Volume declaration and retry policy are appropriate.
Defining the volume with the ConfigMap and setting abackoffLimitof 10 ensures that transient failures can be retried appropriately.build/charts/yorkie-analytics/templates/starrocks/configmap.yaml (1)
1-10: ConfigMap metadata and conditional creation.
The ConfigMap is conditionally created based on the StarRocks enablement flag and includes well-defined metadata. Ensure that any YAML lint warnings (often related to templating syntax or trailing spaces) are handled by your CI/CD tooling.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1)
84-95: Streamlined Kafka CLI arguments construction.
The conditional inclusion of--kafka-addresses,--kafka-topic, and--kafka-write-timeoutusing nestedifstatements simplifies the command-line argument assembly for theyorkiecontainer. Ensure that these values are populated correctly in the correspondingvalues.yamlfile so that the absence of one does not inadvertently remove another necessary parameter.build/docker/analytics/docker-compose.yml (5)
119-121: Centralizing StarRocks host configuration.
DefiningSTARROCKS_MYSQL_HOST=starrocks-feat the beginning of the command block enhances maintainability by avoiding hardcoded hostnames across multiple commands.
123-124: Consistent usage of the environment variable in MySQL checks.
Replacing the hardcoded hostname with$STARROCKS_MYSQL_HOSTin the MySQL commands for checking both the frontend and backend status ensures consistency. The use of double backslashes (e.g.,'show frontends\\G') appears correctly escaped within the YAML context.
128-132: SQL initialization and database check updates.
The commands that load theinit-user-events-db.sqland check for the presence of the newly created database now refer to the centralized environment variable. This change improves consistency across the initialization process.
135-136: Table existence check command update.
Verifying theuser_eventstable using the environment variable in the MySQL command is correctly implemented.
141-146: Routine load creation and verification commands.
The commands executing theinit-routine-load.sqland checking the routine load status are updated with consistent use of$STARROCKS_MYSQL_HOSTand include additional sleep delays to allow the routine load to initialize properly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
build/charts/yorkie-analytics/README.md (2)
1-3: Article and Spelling Correction in IntroductionThe introduction text on line 3 could be improved for clarity. Consider inserting the article "an" before "analytics system" and correcting the spelling of "analyizing" to "analyzing". For instance, modify the line as follows:
-Installs the yorkie-analytics, which provides analytics system with analyz**i**ing usage of yorkie cluster. +Installs the yorkie-analytics, which provides an analytics system with StarRocks and Grafana for analyzing usage of the Yorkie cluster.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: ...ls the yorkie-analytics, which provides analytics system with StarRocks and Grafana for a...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
32-32: Punctuation Suggestion in Dependencies SectionConsider adding a comma after "By default" to improve the readability of the sentence on line 32. For example:
-By default this chart installs additional, dependent charts: +By default, this chart installs additional, dependent charts:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build/charts/yorkie-analytics/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
build/charts/yorkie-analytics/README.md
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: ...ls the yorkie-analytics, which provides analytics system with StarRocks and Grafana for a...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~31-~31: Did you mean: “By default,”?
Context: ...mmand documentation._ ## Dependencies By default this chart installs additional, depende...
(BY_DEFAULT_COMMA)
🪛 markdownlint-cli2 (0.17.2)
build/charts/yorkie-analytics/README.md
62-62: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| See [Customizing the Chart Before Installing](#customizing-the-chart-before-installing) below for configuration options. To see all configurable options with detailed comments: | ||
|
|
||
| ```console | ||
| helm show values yorkie-team/yorkie-analytics | ||
| ``` |
There was a problem hiding this comment.
Link Fragment Verification in Configuration Section
The link "Customizing the Chart Before Installing" on line 62 does not seem to reference any header within the document. Please verify that the corresponding section exists or update the link to point to a valid header.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Link fragments should be valid
null
(MD051, link-fragments)
hackerwins
force-pushed
the
helm-kafka-chart
branch
from
2368ed2 to
89424fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build/charts/yorkie-analytics/README.md (1)
3-3: Refine Language for Clarity and Correct Spelling
The current sentence on line 3 can benefit from improved grammar and spelling. Consider updating it as shown below to enhance clarity and professionalism:-Installs the yorkie-analytics, which provides analytics system with Kafka and StarRocks for analyizing usage of yorkie cluster. +Installs the yorkie-analytics Helm chart, which provides an analytics system with Kafka and StarRocks for analyzing the usage of the Yorkie cluster.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: ...ls the yorkie-analytics, which provides analytics system with Kafka and StarRocks for ana...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
build/charts/yorkie-analytics/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
build/charts/README.md(1 hunks)build/charts/yorkie-analytics/Chart.yaml(1 hunks)build/charts/yorkie-analytics/README.md(1 hunks)build/charts/yorkie-analytics/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- build/charts/yorkie-analytics/Chart.yaml
- build/charts/yorkie-analytics/values.yaml
🧰 Additional context used
🪛 LanguageTool
build/charts/yorkie-analytics/README.md
[uncategorized] ~3-~3: You might be missing the article “an” here.
Context: ...ls the yorkie-analytics, which provides analytics system with Kafka and StarRocks for ana...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
[uncategorized] ~31-~31: Did you mean: “By default,”?
Context: ...mmand documentation._ ## Dependencies By default this chart installs additional, depende...
(BY_DEFAULT_COMMA)
🪛 markdownlint-cli2 (0.17.2)
build/charts/yorkie-analytics/README.md
62-62: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
build/charts/README.md (1)
10-10: New Chart Entry Added
The addition of theyorkie-analyticsHelm chart entry with its clear description is well done. Ensure that the linked documentation in the associated chart’s README continues to match this brief description.build/charts/yorkie-analytics/README.md (1)
62-62: Verify Link Fragment for Customization Section
The link "Customizing the Chart Before Installing" on line 62 does not appear to correspond to any existing header in this document. Please either create a corresponding section or update the link to point to a valid header.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Link fragments should be valid
null(MD051, link-fragments)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml (3)
1-1: Helm Templating Directive & YAMLlint Compatibility
The templating directive on this line ({{- if index .Values "yorkie-analytics" "kafka" "enabled" }}) is standard for Helm charts but may trigger YAML parsing errors (as noted by YAMLlint). Consider adding a YAML lint ignore comment or configuring your linter to skip template directives to avoid false positives.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
11-24: Embedded Bash Script Robustness and Best Practices
The embedded Bash script for initializing Kafka topics is clear and follows the intended flow:
- It constructs the
KAFKA_HOSTusing provided values.- It waits for Kafka to be accessible.
- It creates the Kafka topic if it does not already exist.
Suggestions for Improvement:
- Error Handling: Consider adding checks after executing critical commands (e.g., after listing topics or creating the topic) to handle any failures gracefully. For example, verify the exit status of
kafka-topics.shcommands and exit with an error message if a step fails.- Logging & Verbosity: It might be helpful to redirect error outputs or add additional diagnostic messages for easier debugging during deployments.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: trailing spaces
(trailing-spaces)
15-15: Remove Trailing Whitespace
YAMLlint has flagged trailing whitespace on this line. Removing unnecessary spaces helps maintain a clean and consistent file format.Suggested diff:
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml(1 hunks)build/charts/yorkie-analytics/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build/charts/yorkie-analytics/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml
[error] 15-15: trailing spaces
(trailing-spaces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
build/charts/yorkie-analytics/templates/kafka/configmap.yaml (1)
4-10: Metadata & Label Validation
The metadata block correctly uses template expressions for the resource name, namespace, and labels. Please verify that.Values.nameand.Values.namespaceare defined in your values file, ensuring that the resource naming conventions and label assignments are as intended.
* Add yorkie-analytics Helm chart with Starrocks * Add Kafka yorkie-analytics Helm chart * Enhance Yorkie deployment and Docker Compose for Starrocks integration * Update yorkie-analytics chart to adjust CPU resource requests * Add Kafka topic configuration for partitions and replication factor --------- Co-authored-by: Youngteac Hong <[email protected]>
* Add yorkie-analytics Helm chart with Starrocks * Add Kafka yorkie-analytics Helm chart * Enhance Yorkie deployment and Docker Compose for Starrocks integration * Update yorkie-analytics chart to adjust CPU resource requests * Add Kafka topic configuration for partitions and replication factor --------- Co-authored-by: Youngteac Hong <[email protected]>
What this PR does / why we need it:
Add yorkie-analytics Helm chart with Starrocks and Kafka
Which issue(s) this PR fixes:
Address #1130
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Documentation