Skip to content
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

WIP: feat: refactor hookTrigger by BullMQ #544

Closed
wants to merge 3 commits into from
Closed

Conversation

elrrrrrrr
Copy link
Member

use BullMQ to enhance the availability and performance of hookTrigger, closes #529

  • 🧶 Add AbstractWorker and HookTriggerWorker, which replace the previous polling method used for scheduling tasks.
  • 🤖 Add automatic retry mechanism, where tasks are retried up to 3 times by default. throw exceptions when task errors.
    *❓ Due to the asynchronous nature of worker task scheduling in BullMQ, a separate context (ctx) needs to be created to manage the execution environment.
    *❓ BullMQ does not natively support timeout configuration and does not provide built-in worker concurrency status.

使用 BullMq 来提升 hookTrigger 的可用性, closes #529

  • 🧶 添加 AbstractWorkerHookTriggerWorker ,用以替换定时任务轮询方式
  • 🤖 添加自动重试机制,默认 3 次执行,executeTask 需直接抛出对应异常由调度器进行处理
  • ❓ bullMq 内 woker 异步调度任务,需要独立一个 ctx 上下文
  • ❓ bullmq 默认不支持超时配置,worker 并发状态等信息

@elrrrrrrr elrrrrrrr added the enhancement New feature or request label Jul 4, 2023
@elrrrrrrr elrrrrrrr requested review from fengmk2 and killagu July 4, 2023 17:28
@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
bullmq 4.2.0 eval, network, filesystem, shell +13 8.12 MB manast

@socket-security
Copy link

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue Package Version Note Source
Native code msgpackr-extract 3.0.2

Next steps

What's wrong with native code?

Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs.

Ensure that native code bindings are expected. Consumers may consider pure JS and functionally similar alternatives to avoid the challenges and risks associated with native code bindings.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore msgpackr-extract@3.0.2

@elrrrrrrr elrrrrrrr marked this pull request as ready for review July 4, 2023 17:29
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #544 (b634059) into master (ab2fde7) will increase coverage by 0.16%.
The diff coverage is 98.97%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   97.02%   97.18%   +0.16%     
==========================================
  Files         174      176       +2     
  Lines       16588    16720     +132     
  Branches     2177     2203      +26     
==========================================
+ Hits        16095    16250     +155     
+ Misses        493      470      -23     
Impacted Files Coverage Δ
app/infra/MQAdapter.ts 97.01% <97.01%> (ø)
app.ts 100.00% <100.00%> (ø)
app/common/typing.ts 100.00% <100.00%> (ø)
app/core/entity/Task.ts 100.00% <100.00%> (ø)
app/core/service/CreateHookTriggerService.ts 91.02% <100.00%> (ø)
app/core/service/HookTriggerService.ts 89.18% <100.00%> (+5.40%) ⬆️
app/core/service/TaskService.ts 98.61% <100.00%> (-0.90%) ⬇️
app/core/woker/AbstractWorker.ts 100.00% <100.00%> (ø)
app/core/woker/HookTriggerWorker.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@elrrrrrrr elrrrrrrr changed the title feat: refactor hookTrigger by BullMQ WIP: feat: refactor hookTrigger by BullMQ Jul 5, 2023
@elrrrrrrr elrrrrrrr marked this pull request as draft July 5, 2023 03:13
@elrrrrrrr
Copy link
Member Author

cc @killagu @fengmk2 先帮忙看看
尝试接入后发现有一些问题比较难解决:

  1. bullmq 需要传 redis 的链接配置,不支持注入 redis ,集成模式下,在企业内不一定有明文的配置链接。
  2. bullmq 自己实现消息订阅和任务调度逻辑,缺少 tegg ctx 上下文,手动初始化对象的方式有些丑陋。
  3. 和 rocketMQ 方式差异较大,由于 api 设计原因,比较难抽象成一个统一的的 mq adapter

@@ -60,10 +65,15 @@ export class TaskService extends AbstractService {
return existsTask;
}
await this.taskRepository.saveTask(task);
await this.queueAdapter.push<string>(task.type, task.taskId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我理解发消息是在 queueAdapter 里面包掉的?TaskService 不应该感知这个逻辑。

@@ -60,10 +65,15 @@ export class TaskService extends AbstractService {
return existsTask;
}
await this.taskRepository.saveTask(task);
await this.queueAdapter.push<string>(task.type, task.taskId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我理解发消息是在 queueAdapter 里面包掉的?TaskService 不应该感知这个逻辑。

@fengmk2
Copy link
Member

fengmk2 commented Jul 5, 2023

@elrrrrrrr 直接使用 rocketmq 实现吧,npmmirror 在阿里云部署,支持的。

@elrrrrrrr
Copy link
Member Author

pr 先 close 了,让我们相约 rockect mq

@elrrrrrrr elrrrrrrr closed this Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhook 增加 MQ 来提升成功率
2 participants