fix: Solved the problem of commandstate statistics duplication #3028
Conversation
WalkthroughThe pull request simplifies the macOS build job by removing Homebrew update and extra dependency installation steps, leaving only a reduced set of packages to install. In the configuration file, port offset comments and the rate limiter setting have been updated. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant Conn as PikaClientConn
C->>Conn: Send command with option
Conn->>Conn: Extract command option (opt)
Conn->>Conn: Update cmdstat_map using opt as key
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
Mixficsol
force-pushed
the
config_bug
branch
8 times, most recently
from
76582c5 to
959f8bf
Compare
Mixficsol
force-pushed
the
config_bug
branch
from
959f8bf to
36d4d7b
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pika.yml(1 hunks)conf/pika.conf(2 hunks)src/pika_client_conn.cc(1 hunks)
🔇 Additional comments (3)
.github/workflows/pika.yml (1)
158-158:✅ Verification successful
Simplified macOS dependency installation
This change reduces the number of dependencies installed for macOS builds, which should speed up CI execution.
Let's verify that the removed dependencies ([email protected], protobuf, autoconf, llvm, wget, git) aren't needed elsewhere in the workflow:
🏁 Script executed:
#!/bin/bash # Check for potential usages of the removed dependencies in the workflow echo "Checking for potential usages of removed dependencies..." grep -r "python" --include="*.yml" .github/workflows/ || echo "No direct Python references" grep -r "protobuf" --include="*.yml" .github/workflows/ || echo "No direct protobuf references" grep -r "autoconf" --include="*.yml" .github/workflows/ || echo "No direct autoconf references" grep -r "llvm" --include="*.yml" .github/workflows/ || echo "No direct llvm references" grep -r "wget" --include="*.yml" .github/workflows/ || echo "No direct wget references"Length of output: 1240
Simplified macOS Installation Verified
- Verified that the use of "python" in .github/workflows/codeql.yml is simply part of the language configuration and not an installation dependency.
- Confirmed that "protobuf" and "autoconf" still appear in .github/workflows/pika.yml and .github/workflows/release.yml, which are used for Linux builds.
- Noted that "wget" is referenced only in Linux workflows, and no direct usage of "llvm" was detected.
This confirms that the removal of [email protected], protobuf, autoconf, llvm, wget, and git from the macOS installation is safe and appropriate for speeding up CI on macOS.
conf/pika.conf (2)
6-7:✅ Verification successful
Updated port offset documentation
The documentation now correctly indicates that port+10001 is used for Rsync instead of port+10000.
Let's verify if this port offset is used consistently in the codebase:
🏁 Script executed:
#!/bin/bash # Look for references to port offsets in the code echo "Checking for port offset references in codebase..." grep -r "10000" --include="*.cc" --include="*.h" src/ || echo "No direct references to port+10000" grep -r "10001" --include="*.cc" --include="*.h" src/ || echo "No direct references to port+10001"Length of output: 19249
Port Offset Documentation Update Verified
The changes in
conf/pika.conf(lines 6–7) now correctly document that Rsync usesport+10001instead ofport+10000. A search through the codebase confirmed that there are no direct references to “port+10001,” indicating that this port offset is only used in the documentation as intended.
503-503:❓ Verification inconclusive
Standardized boolean configuration value
Changed boolean representation from
truetoyesfor consistency with other configuration options.Let's check if other boolean configs in the file use the same format:
🏁 Script executed:
#!/bin/bash # Check boolean formats in configuration file echo "Checking boolean formats in pika.conf..." grep -n "true\|false" conf/pika.conf grep -n "yes\|no" conf/pika.conf | head -10Length of output: 1184
Standardized Boolean Value for Rate-Limiter Auto-Tune
The change updating the rate-limiter-auto-tuned configuration to use “yes” shows consistency with several options (e.g., rtc-cache-read and slow-cmd-pool use “yes”/“no”). However, the grep output reveals that a few options (like disable_auto_compactions and wash-data) still use “true/false.” Please verify whether this mixed usage is intentional or if you plan to standardize all boolean values across the file.
| (*cmdstat_map)[opt].cmd_count.fetch_add(1); | ||
| (*cmdstat_map)[opt].cmd_time_consuming.fetch_add(time_stat_->total_time()); |
There was a problem hiding this comment.
Fixed commandstate statistics duplication
This change correctly addresses the problem mentioned in the PR title. Now command statistics are consistently tracked using the command option (opt) instead of the first argument (argv[0]). This ensures proper statistics accounting and aligns with how statistics are tracked elsewhere in the code (see lines 209-210).
fix: #3027
Summary by CodeRabbit