Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a #benchmark-mini command that allows users to trigger performance benchmarks with a shorter test time (10 seconds) compared to the regular #benchmark command. This is useful for quick validation runs without waiting for full benchmark execution.
Changes:
- Added a new GitHub Actions workflow trigger
#benchmark-minithat setssysbench-test-time=10 - Updated the benchmark workflow to pass the
sysbench_test_timeparameter through the workflow chain - Modified the shell scripts to accept and forward the new
sysbenchTestTimeparameter to the Kubernetes job
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .github/workflows/performance-benchmarks-pull-report.yaml | Adds new #benchmark-mini trigger and workflow steps to handle mini benchmarks with configurable test time |
| .github/workflows/k8s-benchmark-latency.yaml | Passes the sysbench test time environment variable to the benchmark script |
| .github/scripts/performance-benchmarking/run-benchmarks.sh | Adds SYSBENCH_TEST_TIME parameter to the template script invocation |
| .github/scripts/performance-benchmarking/get-dolt-dolt-job-json.sh | Updates parameter handling to accept and process the sysbench test time parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| if [ -n "sysbenchTestTime" ]; then | ||
| sysbenchTestTime="\"--sysbenchTestTime=$sysbenchTestTime\"," |
There was a problem hiding this comment.
There's an inconsistency in the flag name used. Line 34 uses --sysbenchTestTime (camelCase), but line 105 uses --sysbench-test-time (kebab-case). These should match. Based on the pattern used in other flags in this file (like --init-big-repo, --noms-bin-format, --issue-number), the kebab-case version on line 105 appears to be correct, so line 34 should use --sysbench-test-time instead.
| sysbenchTestTime="\"--sysbenchTestTime=$sysbenchTestTime\"," | |
| sysbenchTestTime="\"--sysbench-test-time=$sysbenchTestTime\"," |
| id: check | ||
| with: | ||
| trigger: '#benchmark-mini' | ||
| reaction: rocket | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Set Benchmark Mini | ||
| if: ${{ steps.check.outputs.triggered == 'true' }} |
There was a problem hiding this comment.
This step has the same ID as the "Check for Deploy Trigger" step on line 33. Step IDs must be unique within a job. This should be changed to a unique ID like check_mini to avoid conflicts.
| id: check | |
| with: | |
| trigger: '#benchmark-mini' | |
| reaction: rocket | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Set Benchmark Mini | |
| if: ${{ steps.check.outputs.triggered == 'true' }} | |
| id: check_mini | |
| with: | |
| trigger: '#benchmark-mini' | |
| reaction: rocket | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Set Benchmark Mini | |
| if: ${{ steps.check_mini.outputs.triggered == 'true' }} |
| id: check | ||
| with: | ||
| trigger: '#benchmark-mini' | ||
| reaction: rocket | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Set Benchmark Mini | ||
| if: ${{ steps.check.outputs.triggered == 'true' }} |
There was a problem hiding this comment.
This condition references steps.check.outputs.triggered, but because both "Check for Deploy Trigger" and "Check for Deploy Mini Trigger" steps use the same ID (check), this will reference the wrong step. After fixing the duplicate ID on line 46, this should be updated to reference the correct step ID (e.g., steps.check_mini.outputs.triggered).
| id: check | |
| with: | |
| trigger: '#benchmark-mini' | |
| reaction: rocket | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Set Benchmark Mini | |
| if: ${{ steps.check.outputs.triggered == 'true' }} | |
| id: check_mini | |
| with: | |
| trigger: '#benchmark-mini' | |
| reaction: rocket | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Set Benchmark Mini | |
| if: ${{ steps.check_mini.outputs.triggered == 'true' }} |
| if: ${{ needs.validate-commentor.outputs.valid == 'true' }} | ||
| outputs: | ||
| benchmark: ${{ steps.set_benchmark.outputs.benchmark }} | ||
| sysbench-test-time: ${{ steps.set_benchmark.outputs.sysbench-test-time }} |
There was a problem hiding this comment.
The output references steps.set_benchmark.outputs.sysbench-test-time, but this output is only set in the set_benchmark_mini step (line 57), not in the set_benchmark step. When the regular benchmark trigger is used (without the mini variant), this output will be undefined. The reference should either be steps.set_benchmark_mini.outputs.sysbench-test-time or both steps should set this output.
| token: ${{ secrets.REPO_ACCESS_TOKEN }} | ||
| event-type: benchmark-latency | ||
| client-payload: '{"from_server": "dolt", "from_version": "${{ github.sha }}", "to_server": "dolt", "to_version": "${{ steps.comment-branch.outputs.head_sha }}", "mode": "pullRequest", "issue_number": "${{ steps.get_pull_number.outputs.pull_number }}", "init_big_repo": "true", "actor": "${{ github.actor }}", "template_script": "./.github/scripts/performance-benchmarking/get-dolt-dolt-job-json.sh"}' | ||
| client-payload: '{"from_server": "dolt", "from_version": "${{ github.sha }}", "to_server": "dolt", "to_version": "${{ steps.comment-branch.outputs.head_sha }}", "mode": "pullRequest", "issue_number": "${{ steps.get_pull_number.outputs.pull_number }}", "init_big_repo": "true", "actor": "${{ github.actor }}", "sysbench_test_time": "${{ steps.set_benchmark.outputs.sysbench-test-time }}", "template_script": "./.github/scripts/performance-benchmarking/get-dolt-dolt-job-json.sh"}' |
There was a problem hiding this comment.
This references steps.set_benchmark.outputs.sysbench-test-time, but this code is in the performance job, not the check-comments job where the steps are defined. This should reference the job output instead: needs.check-comments.outputs.sysbench-test-time.
| nomsBinFormat="\"--noms-bin-format=$nomsBinFormat\"," | ||
| fi | ||
|
|
||
| if [ -n "sysbenchTestTime" ]; then |
There was a problem hiding this comment.
Missing the dollar sign ($) before the variable name. This should be if [ -n "$sysbenchTestTime" ]; then to properly check if the variable is non-empty. Without the dollar sign, this will always evaluate to true since the literal string "sysbenchTestTime" is always non-empty.
| if [ -n "sysbenchTestTime" ]; then | |
| if [ -n "$sysbenchTestTime" ]; then |
No description provided.