fix: standardize pool parameter naming in AsyncQueue and Kafka facades #1010
Conversation
- Rename parameter from $queue to $pool in AsyncQueue::push() and Kafka facade methods to accurately reflect that these parameters specify connection pools, not queue names - Update AsyncQueue to check both $pool and $queue properties for backward compatibility, with $pool taking precedence - Add @Property annotation for $pool in AsyncQueue docblock - Ensure consistent fallback logic across both facades with 'default' as final fallback - Improve code clarity by using $pool for pool selection and $subPool for batch message grouping in Kafka::sendBatch() This change improves API consistency and makes the codebase more maintainable by using semantically correct parameter names.
|
Warning Rate limit exceeded@huangdijia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
概览AsyncQueue 门面类将 变更清单
预期审查工作量🎯 2 (简单) | ⏱️ ~8 分钟 需额外关注的方面：
可能相关的 PR
诗句
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/facade/src/Kafka.php (1)
18-21: 缺少 @Property 注解与 AsyncQueue facade（src/facade/src/AsyncQueue.php:21）不同，Kafka facade 没有添加
@property null|string $pool注解。为了保持一致性并提供更好的 IDE 支持，建议添加此注解。应用此 diff：
/** * @mixin ProducerManager * @property null|string $queue + * @property null|string $pool */ class Kafka extends Facade
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/facade/src/AsyncQueue.php(1 hunks)src/facade/src/Kafka.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/facade/src/AsyncQueue.php (1)
src/facade/src/Facade.php (1)
Facade(18-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
🔇 Additional comments (3)
src/facade/src/AsyncQueue.php (2)
21-21: 良好的 IDE 支持改进添加
@property null|string $pool注解有助于 IDE 自动补全和类型推断。
33-33: 使用解析后的 pool 值正确假设属性解析顺序问题得到解决，此处的实现是正确的。
src/facade/src/Kafka.php (1)
33-45: 批量发送逻辑实现正确
sendBatch()方法正确地按 pool 对消息进行分组，并为每个 pool 调用相应的生产者。$subPool的引入提高了代码清晰度，避免了变量名称混淆。
| public static function send(ProduceMessage $produceMessage, ?string $pool = null): void | ||
| { | ||
| $queue = (fn ($queue) => $this->queue ?? $queue)->call($produceMessage, $queue); | ||
| self::getProducer($queue)->sendBatch([$produceMessage]); | ||
| $pool ??= (fn () => $this->pool ?? $this->queue ?? 'default')->call($produceMessage); | ||
| self::getProducer($pool)->sendBatch([$produceMessage]); |
There was a problem hiding this comment.
属性检查顺序更合理，但与 AsyncQueue 不一致
Kafka 的属性检查顺序（$this->pool ?? $this->queue）优先检查新的 $pool 属性，这对于标准化工作来说更合理。然而，AsyncQueue（src/facade/src/AsyncQueue.php:31）使用了相反的顺序（$this->queue ?? $this->pool），这会造成两个 facade 之间的不一致。
建议统一两个 facade 的属性检查顺序以提高代码库的一致性。参见对 AsyncQueue.php 的评审意见。
🤖 Prompt for AI Agents
In src/facade/src/Kafka.php around lines 24-27 there is an inconsistency in the
property fallback order compared to AsyncQueue (src/facade/src/AsyncQueue.php
line 31): Kafka uses ($this->pool ?? $this->queue) while AsyncQueue uses
($this->queue ?? $this->pool). Make the facades consistent by changing the Kafka
fallback to the same order used in AsyncQueue (use $this->queue ?? $this->pool
when resolving the default pool/queue) or, alternatively, update both files to
the chosen canonical order; ensure both files use the identical fallback
expression and run tests to confirm no behavior regression.
#1010) * fix: standardize pool parameter naming in AsyncQueue and Kafka facades - Rename parameter from $queue to $pool in AsyncQueue::push() and Kafka facade methods to accurately reflect that these parameters specify connection pools, not queue names - Update AsyncQueue to check both $pool and $queue properties for backward compatibility, with $pool taking precedence - Add @Property annotation for $pool in AsyncQueue docblock - Ensure consistent fallback logic across both facades with 'default' as final fallback - Improve code clarity by using $pool for pool selection and $subPool for batch message grouping in Kafka::sendBatch() This change improves API consistency and makes the codebase more maintainable by using semantically correct parameter names. * fix: add dispatch methods to AMQP, AsyncQueue, and Kafka facades * fix: remove unnecessary suggestion for friendsofhyperf/support in composer.json * fix: standardize parameter naming in Log facade channel method --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
This PR standardizes the parameter naming in the
AsyncQueueandKafkafacades to use$poolinstead of$queue, which more accurately reflects that these parameters specify connection pools rather than queue names.Changes
AsyncQueue facade (src/facade/src/AsyncQueue.php:29):
$queueto$poolinpush()method@property null|string $poolannotation for better IDE support$pooland$queueproperties for backward compatibility$poolparameter → job's$queueproperty → job's$poolproperty → 'default'Kafka facade (src/facade/src/Kafka.php:24, src/facade/src/Kafka.php:33):
$queueto$poolinsend()andsendBatch()methods$subPoolfor clarity in batch message grouping logic$pool→ message's$poolproperty → message's$queueproperty → 'default'Benefits
$queueproperty on jobs/messagesTest plan
$queueproperty setSummary by CodeRabbit
发布说明