fix:macos13 #2937
fix:macos13 #2937
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance the build and test processes for the Pika project and update the Codis-Proxy configuration. The Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
Line range hint 179-182: Remove redundant compiler environment variable
The export CC=... is redundant since you're already setting CMAKE_C_COMPILER in the CMake command.
- export CC=/usr/local/opt/gcc@10/bin/gcc-10
cmake -B build -DCMAKE_C_COMPILER=/usr/local/opt/gcc@10/bin/gcc-10 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccacheLine range hint 89-98: Consider adding cache version to the key
While the caching implementation is correct, consider adding a version to the cache key to allow for easier cache invalidation when needed.
- key: ${{ runner.os }}-rocky-deps-${{ hashFiles('**/CMakeLists.txt') }}
+ key: ${{ runner.os }}-rocky-deps-v1-${{ hashFiles('**/CMakeLists.txt') }}
- key: ${{ runner.os }}-rocky-buildtrees-${{ hashFiles('**/CMakeLists.txt') }}
+ key: ${{ runner.os }}-rocky-buildtrees-v1-${{ hashFiles('**/CMakeLists.txt') }}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/pika.yml (2 hunks)
🔇 Additional comments (3)
.github/workflows/pika.yml (3)
159-159: LGTM: MacOS runner and ccache updates are consistent
The change to macos-13 and corresponding ccache key update are properly aligned.
Also applies to: 172-172
Line range hint 71-74: Verify gcc-toolset-13 compatibility
The use of gcc-toolset-13 is quite recent. Let's ensure it's compatible with the project's requirements.
✅ Verification successful
gcc-toolset-13 is compatible with project requirements
The codebase requires GCC version >= 9 for C++17 support, and gcc-toolset-13 exceeds this minimum requirement. The project's CMake configuration shows proper handling of GCC toolchains and includes the correct link directory for gcc-toolset-13 at /opt/rh/gcc-toolset-13/root/lib/gcc/x86_64-redhat-linux/13.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for specific compiler requirements or version constraints
echo "Searching for compiler requirements..."
rg -i "gcc.*version|compiler.*version|cmake_minimum_required" --type cmake
Length of output: 259
Script:
#!/bin/bash
# Let's check CMakeLists.txt files and build configurations
fd CMakeLists.txt
# Check any compiler requirements in README or docs
fd -e md -e txt | xargs rg -i "gcc|compiler|toolset|requirement"
# Check if there are any specific compiler flags or settings
fd -e cmake -e txt | xargs rg -i "CMAKE_CXX_COMPILER|CMAKE_C_COMPILER|CMAKE_CXX_FLAGS|CMAKE_C_FLAGS"
Length of output: 7266
Line range hint 174-177: Verify Python 3.12 compatibility with the project
While using specific versions helps with reproducibility, Python 3.12 is quite recent. The --overwrite flag suggests potential conflicts.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Python 3.12 installation is appropriate for this project
The project shows no explicit Python version constraints in any configuration files, requirements, or source code. The Python scripts use #!/usr/bin/env python3 shebang and standard library imports that are compatible with Python 3.12. The project primarily uses Python for build and development tools rather than runtime dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Python version requirements in the project
echo "Searching for Python version requirements..."
rg -i "python.*version|requires.*python" --type cmake --type txt
Length of output: 259
Script:
#!/bin/bash
# Check Python version requirements using different approaches
echo "Searching for Python version requirements in various files..."
# Search in requirements files
fd -e txt -e in requirements --exec cat {}
# Search in setup files
fd -e py setup.py --exec cat {}
# Search for Python version patterns in any file
rg -i "python\s*(>=|==|<=|>|<)\s*3\."
# Search for pyproject.toml
fd pyproject.toml --exec cat {}
# Check for Python shebang lines to understand usage
rg -l "^#!.*python"
Length of output: 3303
Script:
#!/bin/bash
# Let's check the Python scripts to understand their usage and dependencies
# Check content of Python files with shebang for version hints
echo "Checking Python files with shebang..."
rg -p "^#!.*python" tools/pika_benchmark/gen_chart.py run_clang_tidy.py
# Look for any import statements that might indicate version requirements
echo "Checking for potentially version-sensitive imports..."
rg -p "^import|^from.*import" tools/pika_benchmark/gen_chart.py run_clang_tidy.py
# Check for any CI/build files that might specify Python requirements
echo "Checking CI files for Python requirements..."
fd -e yml -e yaml --exec cat {}
Length of output: 58688
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pika.yml (1)
Line range hint 1-248: Consider documenting the platform-specific requirements.
While the workflow maintains good consistency across platforms, it would be beneficial to document the platform-specific requirements (like compiler versions, dependencies) in the repository's documentation.
Consider adding a BUILDING.md file that outlines:
- Required compiler versions per platform
- Essential dependencies
- Platform-specific build instructions
Would you like me to help create a template for the BUILDING.md documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/pika.yml (3 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 50-50: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/pika.yml (4)
160-160: LGTM: MacOS runner and ccache updates are properly aligned.
The upgrade to macos-13 and corresponding ccache key update maintain consistency.
Also applies to: 173-173
179-180: LGTM: Added zlib dependency.
Adding zlib explicitly helps ensure build consistency across environments.
Line range hint 123-134: LGTM: Proper caching implementation for Rocky Linux build.
The caching strategy is well implemented:
- Appropriate cache keys based on CMakeLists.txt
- Separate caches for deps and buildtrees
- Proper use of actions/cache@v3
🧰 Tools
🪛 yamllint
[error] 50-50: trailing spaces
(trailing-spaces)
50-50: Verify the dependency preservation approach.
The current approach of copying libz.1.dylib before cleanup needs verification to ensure it doesn't cause runtime issues.
✅ Verification successful
Dependency preservation approach is safe and consistent with build configuration
The verification shows that the approach is safe because:
- The project uses static linking for zlib (
ZLIB_USE_STATIC_LIBS=ONandlibz.ain CMakeLists.txt) - The copied
libz.1.dylibis only used in macOS builds and is preserved before cleanup - The build process and tests (including integration tests) run successfully after the cleanup step
- No other dynamic library dependencies are affected by the cleanup process
The preservation of libz.1.dylib is a macOS-specific requirement and doesn't impact the runtime behavior since the main binary is statically linked with zlib.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if libz is referenced in CMake files
echo "Checking libz references in CMake files..."
rg -l "libz|ZLIB" . --type cmake
# Check for dynamic library dependencies
echo "Checking for other potential dynamic library dependencies..."
rg -l "\.dylib" . --type cmake
Length of output: 454
Script:
#!/bin/bash
# Check the content of CMake files for ZLIB usage
echo "Checking ZLIB usage in CMakeLists.txt files..."
rg "ZLIB|libz" -A 5 -B 5 ./CMakeLists.txt ./src/storage/CMakeLists.txt
# Check the workflow file for context around libz.1.dylib copy
echo "Checking workflow context..."
cat .github/workflows/pika.yml
# Check for any other dylib related operations
echo "Checking for other dylib operations..."
rg "\.dylib" .github/workflows/
Length of output: 11286
🧰 Tools
🪛 yamllint
[error] 50-50: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pika.yml (2)
197-200: Review the libz.1.dylib dependency handling.The need to copy libz.1.dylib suggests a potential dependency management issue:
- Why is this library specifically needed after cleanup?
- Consider documenting the reason for this requirement
- Fix the trailing space on line 199
Consider these alternatives:
- Include libz as a proper dependency in the build system
- Use dynamic library path settings instead of manual copies
Fix the trailing space:
- rm -rf ./deps + rm -rf ./deps🧰 Tools
🪛 yamllint
[error] 199-199: trailing spaces
(trailing-spaces)
Line range hint
92-103: Optimize cache configuration for better hit rates.The current cache configuration could be improved:
- Cache key only considers CMakeLists.txt changes
- No fallback strategy with restore-keys
Consider this enhanced cache configuration:
- uses: actions/cache@v3 with: path: ${{ github.workspace }}/deps - key: ${{ runner.os }}-rocky-deps-${{ hashFiles('**/CMakeLists.txt') }} + key: ${{ runner.os }}-rocky-deps-v1-${{ hashFiles('**/CMakeLists.txt', '**/vcpkg.json') }} + restore-keys: | + ${{ runner.os }}-rocky-deps-v1- - uses: actions/cache@v3 with: path: ${{ github.workspace }}/buildtrees - key: ${{ runner.os }}-rocky-buildtrees-${{ hashFiles('**/CMakeLists.txt') }} + key: ${{ runner.os }}-rocky-buildtrees-v1-${{ hashFiles('**/CMakeLists.txt', '**/vcpkg.json') }} + restore-keys: | + ${{ runner.os }}-rocky-buildtrees-v1-This change:
- Adds version prefix for cache invalidation
- Includes vcpkg.json in hash if present
- Provides fallback with restore-keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(3 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 199-199: trailing spaces
(trailing-spaces)
.github/workflows/pika.yml
Outdated
| - name: Uninstall Homebrew | ||
| run: | | ||
| if command -v brew > /dev/null; then | ||
| /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/uninstall.sh)" | ||
| fi |
There was a problem hiding this comment.
💡 Codebase verification
Homebrew uninstall step must be removed or relocated
The verification confirms that Homebrew is required for installing critical dependencies (python, gcc, autoconf, etc.) after the uninstall step. This will cause the workflow to fail since these dependencies cannot be installed without Homebrew.
Suggested fixes:
- Remove the Homebrew uninstall step entirely, or
- Move it to the end of the workflow if cleanup is necessary
🔗 Analysis chain
Verify the Homebrew uninstall step's impact.
The Homebrew uninstall step could be problematic since subsequent steps rely on Homebrew for installing dependencies. Consider:
- Moving this step after the dependency installation
- Or removing it if it's not strictly necessary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if subsequent steps use Homebrew
rg -A 5 'brew' .github/workflows/pika.yml
Length of output: 861
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pika.yml (1)
Line range hint
1-254: Consider matrix strategy for OS versionsThe workflow effectively tests across different platforms, but consider using GitHub Actions matrix strategy to reduce duplication and make it easier to add/remove OS versions in the future.
Example matrix configuration:
strategy: matrix: os: [ubuntu-latest, rockylinux:9, macos-13] include: - os: rockylinux:9 container: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(4 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 194-194: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/pika.yml (4)
159-160: LGTM: macOS runner update
The update to macos-13 aligns with GitHub's latest runner offerings and ensures better long-term support.
173-173: LGTM: Cache key update
Appropriate update of the ccache key to match the new macOS version, ensuring proper cache isolation between different macOS versions.
207-220: Verify integration test paths
The working directory changes for integration tests look correct, but let's verify the path structure.
✅ Verification successful
Integration test paths are correctly structured and accessible
All referenced shell scripts exist at the expected locations in the tests/integration/ directory:
start_master_and_slave.shstart_codis.shintegrate_test.sh
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the integration test directory structure
fd -t f -e sh 'start_.*\.sh|integrate_test\.sh' tests/integration/
# Verify referenced scripts exist
test -f tests/integration/start_master_and_slave.sh || echo "Missing start_master_and_slave.sh"
test -f tests/integration/start_codis.sh || echo "Missing start_codis.sh"
test -f tests/integration/integrate_test.sh || echo "Missing integrate_test.sh"
Length of output: 324
192-195: Fix trailing whitespace and verify libz.1.dylib requirement
The cleanup steps have been updated to preserve the required libz.1.dylib, but there's a trailing whitespace on line 194.
Apply this diff to fix the whitespace:
- rm -rf ./deps
+ rm -rf ./deps✅ Verification successful
Based on the search results, I can see that zlib is a required dependency in the project, used by both RocksDB and the main project. The CMake files show that zlib is built statically and the library is installed to the deps/lib directory. The workflow step that copies libz.1.dylib is specifically for macOS (indicated by the .dylib extension), where dynamic libraries are needed for runtime.
Fix trailing whitespace and keep libz.1.dylib preservation
The trailing whitespace should be removed, and the preservation of libz.1.dylib is indeed necessary for macOS builds.
Apply this diff to fix the whitespace:
- rm -rf ./deps
+ rm -rf ./deps🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if libz.1.dylib is actually required
rg -l 'libz\.1\.dylib' --type cpp --type c
Length of output: 44
Script:
#!/bin/bash
# Let's check for any references to libz in the codebase more broadly
rg -l "libz" --type-add 'build:*.{cmake,sh,yml,yaml}' --type build --type cpp --type c
# Also check for zlib references which might indicate dependency
rg -l "zlib" --type-add 'build:*.{cmake,sh,yml,yaml}' --type build --type cpp --type c
# Check CMakeLists.txt files specifically for zlib/libz dependencies
fd CMakeLists.txt --exec cat {}
Length of output: 41313
🧰 Tools
🪛 yamllint
[error] 194-194: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(3 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 194-194: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/pika.yml (3)
159-160: LGTM: Appropriate upgrade to macOS 13
The upgrade to macos-13 is a good proactive change as GitHub is phasing out older macOS versions.
173-173: LGTM: Cache key properly updated
The cache key has been correctly updated to match the new macOS version, preventing potential cache conflicts.
Line range hint 82-83: LGTM: Appropriate package management for Rocky Linux
The switch to dnf is correct for Rocky Linux 9, and all necessary build dependencies are included.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pikatests.sh (1)
Line range hint
3-9: Update cleanup function to remove integration test binary.The cleanup function should be updated to remove all test artifacts, including the newly added integration test binary.
Apply this change to maintain test isolation:
function cleanup() { rm -rf ./log[0-9]* rm -rf ./db[0-9]* rm -rf dbsync/ rm src/redis-server + rm -f tests/integration/pika }.github/workflows/pika.yml (1)
192-195: Document libz.1.dylib preservation and fix whitespaceWhile copying
libz.1.dylibbefore cleanup is correct, consider adding a comment explaining why this library needs to be preserved. Also, there's a trailing whitespace that should be removed.Apply this diff to fix the whitespace and add documentation:
# Preserve libz.1.dylib as it's required for runtime dependencies cp deps/lib/libz.1.dylib . cp deps/lib/libz.1.dylib tests/integration/ -rm -rf ./deps +rm -rf ./deps rm -rf ./buildtree🧰 Tools
🪛 yamllint
[error] 194-194: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/pika.yml(4 hunks)pikatests.sh(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 194-194: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
pikatests.sh (1)
47-49: Verify integration test directory structure and dependencies.
Since this change is related to workflow modifications that copy libz.1.dylib, we should verify the integration test setup is complete.
Let's check the integration test directory structure and related workflow changes:
✅ Verification successful
Integration test setup and workflow changes are properly aligned
The integration test directory structure is complete and matches the workflow changes:
- Integration tests are properly set up in
tests/integration/with all necessary test files - The workflow correctly copies
libz.1.dylibto both root andtests/integration/directories - The test setup script
pikatests.shcorrectly copies the binary to the required test locations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration test setup and related workflow changes
# Check if integration test directory exists and list its contents
echo "Checking integration test directory structure:"
find tests/integration -type f -ls || echo "Integration test directory not found"
# Check for related workflow changes
echo -e "\nChecking related workflow changes:"
rg -A 5 "libz.1.dylib" .github/workflows/
Length of output: 3723
.github/workflows/pika.yml (3)
159-160: LGTM: macOS runner update
The update to macos-13 is appropriate as it's the latest stable version supported by GitHub Actions.
173-173: Update ccache key for consistency
The ccache key has been correctly updated to match the new macOS version.
207-211: Verify working directory changes
The working directory changes for test execution look correct, but let's verify that all required files are accessible from the new paths.
Also applies to: 214-218
✅ Verification successful
Working directory and test files are correctly configured
The verification confirms that both required shell scripts (start_master_and_slave.sh and integrate_test.sh) are present in the tests/integration/ directory, which aligns with the working directory changes in the workflow. The file paths and permissions setup in the workflow are correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of required test files in the integration directory
echo "Checking for required test files..."
fd -t f -e sh "start_master_and_slave.sh|integrate_test.sh" tests/integration/
Length of output: 241
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
codis/config/proxy.toml (1)
108-111: LGTM on command categorization, but fix formatting.The categorization of Redis commands into quick (get,set) and slow (mget,mset) lists is logically sound as it separates single-key from multi-key operations. However, there's an inconsistency in formatting.
Apply this diff to maintain consistent formatting:
quick_cmd_list = "get,set" -slow_cmd_list = "mget, mset" +slow_cmd_list = "mget,mset"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/pika.yml(4 hunks)codis/config/proxy.toml(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 194-194: trailing spaces
(trailing-spaces)
[error] 214-214: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
codis/config/proxy.toml (1)
108-111: Verify Redis command compatibility.
Since these command lists affect proxy behavior and connection pooling (via backend_primary_quick and backend_replica_quick settings), ensure all listed commands are supported by your Redis version and Codis implementation.
Run this script to check command references in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
Redis commands in configuration are properly supported
The codebase shows comprehensive support for the specified Redis commands:
get,set: Supported as quick commands with dedicated implementations intools/benchmark_client/benchmark_client.ccmget,mset: Supported as slow commands with implementations intools/codis2pika/scripts/commands/
The command categorization in proxy.toml aligns with Codis's connection pooling design:
- Quick commands (
get,set) usebackend_primary_quickpool - Slow commands (
mget,mset) use regular connection pool
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Redis command handling in the codebase
# Search for command handling implementations
echo "Searching for command handling code..."
rg -i "(?:get|set|mget|mset).*command"
# Search for any command validation or restrictions
echo "Searching for command validation code..."
rg -i "(?:quick|slow).*cmd.*list"
Length of output: 94512
.github/workflows/pika.yml (4)
159-160: LGTM: MacOS runner and cache key updates are consistent.
The change from macos-12 to macos-13 and the corresponding ccache key update are appropriate.
Also applies to: 173-173
Line range hint 175-179: Existing issue: Homebrew uninstall timing
Referencing existing review comment about the Homebrew uninstall step's impact on dependency installation.
195-195: LGTM: Correct cleanup path.
The path correction from buildtree to buildtrees aligns with the actual directory name used in the build process.
207-214: LGTM: Improved test execution reliability.
The working directory changes and path updates ensure that integration tests are executed from the correct location consistently.
Also applies to: 217-219
🧰 Tools
🪛 yamllint
[error] 214-214: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/integration/start_codis.sh (3)
Line range hint
1-31: Improve script portability and error handling.The script has potential portability and robustness issues:
- The
sedcommands use macOS-specific syntax (-i ''), which won't work on Linux.- Directory creation doesn't check for existing directories or handle errors.
Consider applying these improvements:
- mkdir codis_data_1 - mkdir codis_data_2 + mkdir -p codis_data_1 codis_data_2 - sed -i '' -e 's|databases : 1|databases : 2|' ... + # Use portable sed syntax + if [[ "$OSTYPE" == "darwin"* ]]; then + sed -i '' -e 's|databases : 1|databases : 2|' ... ./pika_8000.conf + else + sed -i -e 's|databases : 1|databases : 2|' ... ./pika_8000.conf + fi🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
Line range hint
36-999: Improve component startup reliability and error handling.The current startup process has several reliability concerns:
- Fixed sleep durations don't guarantee component readiness
- No verification of successful component startup
- No error handling for configuration commands
Consider implementing a more robust startup sequence:
# Function to check if a service is ready wait_for_service() { local service_name="$1" local max_attempts=30 local endpoint="$2" echo "Waiting for $service_name to be ready..." for ((i=1; i<=max_attempts; i++)); do if curl -s "$endpoint" >/dev/null; then echo "$service_name is ready" return 0 fi sleep 1 done echo "$service_name failed to start" return 1 } # Start dashboard with verification ./admin/codis-dashboard-admin.sh start wait_for_service "dashboard" "$CODIS_DASHBOARD_ADDR" || exit 1 # Start proxy with verification ./admin/codis-proxy-admin.sh start wait_for_service "proxy" "127.0.0.1:19000" || exit 1 # Configure groups with error handling configure_group() { local gid="$1" local addr="$2" if ! ./bin/codis-admin --dashboard=$CODIS_DASHBOARD_ADDR --create-group --gid="$gid"; then echo "Failed to create group $gid" return 1 fi if ! ./bin/codis-admin --dashboard=$CODIS_DASHBOARD_ADDR --group-add --gid="$gid" --addr="$addr"; then echo "Failed to add server to group $gid" return 1 fi return 0 } configure_group 1 "$CODIS_GROUP_1_MASTER" || exit 1 configure_group 2 "$CODIS_GROUP_2_MASTER" || exit 1🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
Line range hint
3-7: Consider implementing proper cleanup handling.The commented-out cleanup code suggests these operations might be necessary. Consider implementing proper cleanup handling:
- Uncomment and move cleanup operations to a separate function
- Add trap handlers for script termination
Here's a suggested implementation:
cleanup() { echo "Cleaning up..." pkill -9 pika pkill -9 codis rm -rf /tmp/codis codis_data_1 codis_data_2 } # Set up trap handlers trap cleanup EXIT trap 'trap - EXIT; cleanup; exit 1' INT TERM🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/integration/start_codis.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
tests/integration/start_codis.sh
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
tests/integration/start_codis.sh
Outdated
|
|
||
| cd ../ | ||
| chmod +x ./codis | ||
| cd codis |
There was a problem hiding this comment.
Add error handling for directory operations and clarify build process.
The current directory navigation and build process lacks error handling and clarity:
cdcommands should handle failures- The purpose of setting executable permissions on the
codisdirectory is unclear - Build process could fail silently
Consider this safer implementation:
- cd ../
- chmod +x ./codis
- cd codis
- make
+ # Navigate to codis directory and build
+ cd ../ || { echo "Failed to navigate to parent directory"; exit 1; }
+ if [[ ! -d "./codis" ]]; then
+ echo "Error: codis directory not found"
+ exit 1
+ fi
+ cd codis || { echo "Failed to navigate to codis directory"; exit 1; }
+ if ! make; then
+ echo "Error: build failed"
+ exit 1
+ fiCommittable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(4 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 194-194: trailing spaces
(trailing-spaces)
[error] 214-214: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pika.yml (1)
159-160: LGTM: macOS runner and cache key updates are correct.
The update to macos-13 and corresponding ccache key change are appropriate, ensuring proper cache isolation between different macOS versions.
Also applies to: 173-173
* fix:macos13 --------- Co-authored-by: chejinge <chejinge@360.cn>
* fix:macos13 --------- Co-authored-by: chejinge <chejinge@360.cn>
Summary by CodeRabbit
New Features
pikabinary in the integration tests.Bug Fixes