-
-
Notifications
You must be signed in to change notification settings - Fork 27
refactor: extract common dispatch logic to AbstractPendingDispatch #1018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Create AbstractPendingDispatch base class with Conditionable trait - Move __destruct logic to abstract dispatch() method - Add defer() method for deferred dispatch execution - Refactor PendingAmqpProducerMessageDispatch, PendingAsyncQueueDispatch, and PendingKafkaProducerMessageDispatch to extend abstract class This improves code reusability and maintains consistency across different message dispatch implementations.
Walkthrough新增抽象基类 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pending as 子类 PendingDispatch
participant Abstract as AbstractPendingDispatch
participant Scheduler as 延迟调度器
rect rgb(200,220,240)
Note over Client,Abstract: 场景A — 未调用 defer(析构触发)
Client->>Pending: 创建并配置实例
Client->>Client: 超出作用域 / 销毁
Pending->>Abstract: __destruct()
Abstract->>Pending: 调用 dispatch()
Pending->>Pending: 执行发送/入队/produce 等逻辑
end
rect rgb(240,220,200)
Note over Client,Scheduler: 场景B — 调用 defer(延迟调度)
Client->>Pending: 创建并配置实例
Client->>Abstract: 调用 defer()
Abstract->>Abstract: 设置 defer 标志
Abstract->>Scheduler: 注册延迟回调(稍后触发)
Client->>Client: 超出作用域 / 销毁
Pending->>Abstract: __destruct()
Abstract->>Abstract: 检查 defer 标志,跳过 dispatch
Scheduler->>Pending: 延迟回调触发 dispatch()
Pending->>Pending: 执行发送/入队/produce 等逻辑
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/support/src/Bus/PendingAsyncQueueDispatch.php (1)
18-18: 继承抽象基类并集中 dispatch 逻辑是合理的
- 通过继承
AbstractPendingDispatch把析构触发的派发逻辑收敛到了统一模板,dispatch()内只关心具体的 async-queue 实现,结构清晰、易维护。ApplicationContext::getContainer()->get(DriverFactory::class)->get($this->pool ?? 'default')->push($this->job, $this->delay)与既有 async-queue 使用方式一致,默认 pool 为default也与其它地方约定统一,看不到行为上的回归风险。可以后续视情况考虑的小优化(非必须):
- 如果希望
dispatch()的返回值语义更明确,可以在抽象类和所有子类上加上: void返回类型声明,以便静态分析工具更友好(前提是确认没有其他子类会返回值)。Also applies to: 46-52
src/support/src/Bus/PendingKafkaProducerMessageDispatch.php (1)
24-24: Kafka PendingDispatch 与抽象基类的整合基本合理
- 改为继承
AbstractPendingDispatch后,通过统一的dispatch()模板在析构时发送 Kafka 消息,链式配置方法(onPool、headers、key/value)保持不变,整体对外 API 看起来是向后兼容的。ApplicationContext::getContainer()->get(ProducerManager::class)->getProducer($this->pool ?? 'default')->sendBatch([$this->message])的默认 pool 行为与 helpers 里 Kafka dispatch 的默认值一致,符合预期。可选的小提升(非必须):
- 如需完全对齐
Kafka::sendBatch()中对消息 pool/queue 的推断($pool ?? $this->pool ?? $this->queue ?? 'default'),可以考虑把这段 pool 选择逻辑抽成一个共享方法以复用,避免未来两处逻辑渐行渐远。Also applies to: 57-62
src/support/src/Bus/PendingAmqpProducerMessageDispatch.php (1)
21-21: AMQP PendingDispatch 的重构与模板方法模式契合良好
- 通过继承
AbstractPendingDispatch并把 AMQP 发送逻辑移动到dispatch(),将原本散落在析构中的行为集中到一个可测试的地方,同时保持了对外链式配置 API 的兼容性。dispatch()中按需设置 pool / routingKey / exchange,然后通过容器获取Producer调用produce($this->message, $this->confirm, $this->timeout),和既有 AMQP 使用模式一致,行为上看不到明显回归风险。可选的细节优化(非必须):
- 当前使用的短路写法:
$this->pool && $this->message->setPoolName($this->pool);$this->routingKey && $this->message->setRoutingKey($this->routingKey);$this->exchange && $this->message->setExchange($this->exchange);
会在值为''或[]等“假值”时跳过设置。若业务上这些值也被视为有效配置,可以考虑改成显式判null,例如:以避免Truthy/Falsy 语义带来的歧义。if ($this->pool !== null) { $this->message->setPoolName($this->pool); }Also applies to: 82-90
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/support/src/Bus/AbstractPendingDispatch.php(1 hunks)src/support/src/Bus/PendingAmqpProducerMessageDispatch.php(2 hunks)src/support/src/Bus/PendingAsyncQueueDispatch.php(2 hunks)src/support/src/Bus/PendingKafkaProducerMessageDispatch.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/support/src/Bus/AbstractPendingDispatch.php (3)
src/support/src/Bus/PendingAmqpProducerMessageDispatch.php (1)
dispatch(82-90)src/support/src/Bus/PendingAsyncQueueDispatch.php (1)
dispatch(46-52)src/support/src/Bus/PendingKafkaProducerMessageDispatch.php (1)
dispatch(57-63)
src/support/src/Bus/PendingAsyncQueueDispatch.php (2)
src/support/src/Bus/AbstractPendingDispatch.php (2)
AbstractPendingDispatch(16-39)dispatch(38-38)src/support/src/Functions.php (1)
dispatch(33-45)
src/support/src/Bus/PendingKafkaProducerMessageDispatch.php (6)
src/support/src/Bus/AbstractPendingDispatch.php (2)
AbstractPendingDispatch(16-39)dispatch(38-38)src/support/src/Bus/PendingAmqpProducerMessageDispatch.php (1)
dispatch(82-90)src/support/src/Bus/PendingAsyncQueueDispatch.php (1)
dispatch(46-52)src/support/src/Functions.php (1)
dispatch(33-45)src/helpers/src/Functions.php (1)
dispatch(197-224)src/facade/src/Kafka.php (1)
sendBatch(40-53)
src/support/src/Bus/PendingAmqpProducerMessageDispatch.php (4)
src/support/src/Bus/AbstractPendingDispatch.php (2)
AbstractPendingDispatch(16-39)dispatch(38-38)src/support/src/Bus/PendingAsyncQueueDispatch.php (1)
dispatch(46-52)src/support/src/Bus/PendingKafkaProducerMessageDispatch.php (1)
dispatch(57-63)src/support/src/Functions.php (1)
dispatch(33-45)
⏰ 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.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 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.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 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.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.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
🔇 Additional comments (1)
src/support/src/Bus/AbstractPendingDispatch.php (1)
16-38: 需要进一步验证defer()函数的来源。让我检查文件头部的use语句和 composer 依赖。现在检查文件头部的导入情况和命名空间:
验证完成:
defer()函数来源确认,代码无问题经过验证,
defer()是 Hyperf 框架提供的全局函数,用于在协程结束时执行延迟代码,代码中第 33 行的使用是正确的。该抽象类的生命周期设计清晰、逻辑简洁,当前实现不存在功能缺陷。建议(可选):
- 如果要严格保护生命周期语义,可将
__destruct()标记为final- 可在
abstract protected function dispatch();上添加显式返回类型: void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/composer.json(1 hunks)
⏰ 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). (11)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (1)
src/sentry/composer.json (1)
39-47: 确认 hyperf/engine 是否应为可选依赖还是必需依赖。该条目被添加到 "suggest" 部分作为可选依赖。请确认:
- 新的 AbstractPendingDispatch 实现是否真的只在可选情况下需要 hyperf/engine?
- 如果使用了延迟调度功能是否一定需要此依赖?
如果没有 hyperf/engine 会导致功能故障,应考虑将其移至 "require" 部分。
| "hyperf/amqp": "Required to use the amqp event (~3.1.9).", | ||
| "hyperf/crontab": "Required to use the crontab event (~3.1.7).", | ||
| "hyperf/database": "Required to use the crontab event (~3.1.0).", | ||
| "hyperf/engine": "Required to use the coroutine engine (^2.15.0).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Verification inconclusive
验证 hyperf/engine 包版本约束的有效性。
新增的 "hyperf/engine" 可选依赖版本约束为 "^2.15.0"。考虑到此更改与 PR 中引入的 AbstractPendingDispatch 抽象基类的延迟调度功能相关,请确认该版本号是否有效且存在。
建议运行以下命令以验证该依赖的可用性和版本信息:
🏁 Script executed:
#!/bin/bash
# Description: Verify hyperf/engine package existence and latest versions
# Check PackageST/Packagist for hyperf/engine details
curl -s https://repo.packagist.org/p/hyperf/engine.json | jq '.packages["hyperf/engine"] | keys' | head -20Length of output: 57
版本约束 "^2.15.0" 指向不存在的包版本
hyperf/engine 的最新可用版本是 v2.14.0(2025年4月13日发布)。composer.json 中建议的版本约束 "^2.15.0" 引用的是尚不存在的版本,这将导致 composer 安装时失败。
建议改为 "^2.14.0" 或移除此约束,直到版本 2.15.0 正式发布。
🤖 Prompt for AI Agents
In src/sentry/composer.json around line 44, the version constraint "^2.15.0" for
hyperf/engine references a non-existent release and will break composer
installs; change the constraint to a valid published version (for example
"^2.14.0") or remove the hyperf/engine requirement until v2.15.0 is officially
released, then run composer validate and composer update to confirm the manifest
and dependency resolution succeed.
Summary
This PR refactors the dispatch classes to extract common functionality into an abstract base class.
Changes
Created AbstractPendingDispatch - New abstract base class that:
Refactored dispatch classes to extend AbstractPendingDispatch:
Each concrete class now:
Benefits
Testing
The refactoring maintains backward compatibility - existing usage patterns remain unchanged.
Summary by CodeRabbit
新功能
改进
维护