Added casting, and error checking for pbench config script #98
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe shell script now includes colorized logging helpers, integer-cast RAM calculation, VARIANT_TYPE validation with GPU-specific vCPU configuration, and post-generation error detection with status messaging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes are straightforward enhancements: addition of helper functions and color constants, a single calculation improvement, and conditional logic for variant handling. While multiple types of changes are present, they follow consistent patterns and add minimal complexity to the existing flow. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
presto/scripts/generate_presto_config.sh (2)
19-22: Remove unusedYELLOWcolor constant.The static analysis tool correctly identifies that
YELLOWis never used in the script. Remove it to avoid confusion and maintain clean code.RED='\033[0;31m' GREEN='\033[0;32m' -YELLOW='\033[1;33m' NC='\033[0m' # No Color
83-86: Remove unreachableexit 1statement.The
exit 1on line 85 is unreachable becauseecho_error(defined on lines 24–27) already exits the script with status 1. Remove the redundant statement:if ../../pbench/pbench genconfig -p params.json -t template generated 2>&1 | grep '\{\"level":"error"'; then echo_error "ERROR in pbench genconfig. Configs were not generated successfully" - exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
presto/scripts/generate_presto_config.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
presto/scripts/generate_presto_config.sh
[warning] 21-21: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (5)
presto/scripts/generate_presto_config.sh (5)
24-31: Well-designed logging helpers.The
echo_errorandecho_successfunctions provide consistent, colorized output and proper exit handling. Clean implementation.
41-41: Integer casting for memory value is correct.The
int()cast in the awk expression properly addresses the original issue of floats breaking config parsing. The calculation correctly converts bytes to GB.
47-50: Solid validation of VARIANT_TYPE.The regex check ensures only valid variant types are accepted, preventing misconfigurations. Clear error messaging.
51-55: GPU-specific vCPU configuration is appropriate.The logic correctly assigns fewer vCPUs for GPU variants while preserving full CPU count for CPU/Java variants.
83-86: Verify that pbench error detection covers all failure modes.The current error detection pattern
\{\"level":"error"assumes pbench always outputs JSON-formatted errors. If pbench can fail in other ways (exit with error code but no error JSON output), those failures would be missed.Please confirm: does pbench consistently format errors as JSON with
{"level":"error", or are there other failure modes that should be caught?
simoneves
left a comment
simoneves
left a comment
There was a problem hiding this comment.
Approved, but maybe the fancy logging is a little PR-creep-y?
| function echo_success { | ||
| echo -e "${GREEN}$1${NC}" | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice, but maybe this stuff could move to the shared-parser PR or some other all-scripts clean-up, to avoid proliferating custom code?
There was a problem hiding this comment.
Also, CodeRabbit is not wrong...
There was a problem hiding this comment.
It's actually already in a PR I have here: #60
Folks wanted me to wait until things are a bit less in flux before landing it though, as it touches a lot of our scripts. My intention is to land this with the helper functions, and then I can replace them easily when PR 60 lands by remove the functions and sourcing the shared helper script.
There was a problem hiding this comment.
My intention is to land this with the helper functions, and then I can replace them easily when PR 60 lands by remove the functions and sourcing the shared helper script.
Fair enough!
…i/velox-testing into misiug/PatchPbenchScript
The pbench config script was swallowing errors such that it was not clear when it failed (and the presto start scripts would continue even if it had, using/creating invalid config files). With this patch we cast mem values to int (the original failure was from floats being passed in and failing parsing), we check the pbench output for errors, and will have the script return an error value if the config failed (thereby halting the start* scripts if they call it and fail).
Summary by CodeRabbit
New Features
Bug Fixes
Chores