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

feat: use sort set to impl queue #277

Merged
merged 2 commits into from Aug 4, 2022
Merged

feat: use sort set to impl queue #277

merged 2 commits into from Aug 4, 2022

Conversation

killagu
Copy link
Contributor

@killagu killagu commented Aug 4, 2022

Use sort set to keep queue in order and keep same value only insert once

Use sort set to keep queue in order and keep same value only insert once
*/
async push<T>(key: string, item: T): Promise<boolean> {
const score = await this.redis.incr(this.getQueueScoreName(key));
const res = await this.redis.zadd(this.getQueueName(key), score, JSON.stringify(item));
Copy link
Member

Choose a reason for hiding this comment

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

有什么文章看看是什么原理么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fengmk2 fengmk2 added bug Something isn't working enhancement New feature or request labels Aug 4, 2022
@fengmk2
Copy link
Member

fengmk2 commented Aug 4, 2022

除了保证同步任务不重复,后面还是需要考虑真的有并发同步执行的时候,数据更新的原子性,目前可以先忽略。

@fengmk2
Copy link
Member

fengmk2 commented Aug 4, 2022

oss 并发写入按道理是不会有问题的。

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #277 (b40d954) into main (269cbf1) will decrease coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
- Coverage   97.43%   97.02%   -0.41%     
==========================================
  Files         119      119              
  Lines        3661     3665       +4     
  Branches      330      330              
==========================================
- Hits         3567     3556      -11     
- Misses         94      109      +15     
Impacted Files Coverage Δ
app/core/service/TaskService.ts 97.05% <ø> (-1.48%) ⬇️
app/infra/QueueAdapter.ts 100.00% <100.00%> (ø)
config/config.default.ts 75.00% <0.00%> (-15.63%) ⬇️
app/infra/NFSClientAdapter.ts 74.07% <0.00%> (-14.82%) ⬇️
app/common/adapter/binary/PlaywrightBinary.ts 94.11% <0.00%> (-5.89%) ⬇️
app/port/controller/BinarySyncController.ts 92.30% <0.00%> (-5.13%) ⬇️
app/common/adapter/NFSAdapter.ts 96.66% <0.00%> (-3.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

* If queue not has the same item, return true
*/
async push<T>(key: string, item: T): Promise<boolean> {
const score = await this.redis.incr(this.getQueueScoreName(key));
Copy link
Contributor

Choose a reason for hiding this comment

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

First, Redis is not a good choice for the task queue. I think it would be better to use MQ or other tools.

Second, maybe you can use use Lua script to keep the command atomic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MQ is good for queue, but we choice less infrastructure dependency so use the redis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, lua script is better, we should use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And infra can be replace with other implement. It's ok to use mq impl the new queue adapter.

@killagu
Copy link
Contributor Author

killagu commented Aug 4, 2022

According to the redis doc.

Prior versions of Redis made scripting available only via the EVAL command, which allows a Lua script to be sent for execution by the server. The core use cases for Eval Scripts is executing part of your application logic inside Redis, efficiently and atomically. Such script can perform conditional updates across multiple keys, possibly combining several different data types.

Using EVAL requires that the application sends the entire script for execution every time. Because this results in network and script compilation overheads, Redis provides an optimization in the form of the EVALSHA command. By first calling SCRIPT LOAD to obtain the script's SHA1, the application can invoke it repeatedly afterward with its digest alone.

By design, Redis only caches the loaded scripts. That means that the script cache can become lost at any time, such as after calling SCRIPT FLUSH, after restarting the server, or when failing over to a replica. The application is responsible for reloading scripts during runtime if any are missing. The underlying assumption is that scripts are a part of the application and not maintained by the Redis server.

This approach suits many light-weight scripting use cases, but introduces several difficulties once an application becomes complex and relies more heavily on scripting, namely:

All client application instances must maintain a copy of all scripts. That means having some mechanism that applies script updates to all of the application's instances.
Calling cached scripts within the context of a transaction increases the probability of the transaction failing because of a missing script. Being more likely to fail makes using cached scripts as building blocks of workflows less attractive.
SHA1 digests are meaningless, making debugging the system extremely hard (e.g., in a MONITOR session).
When used naively, EVAL promotes an anti-pattern in which scripts the client application renders verbatim scripts instead of responsibly using the KEYS and ARGV Lua APIs.
Because they are ephemeral, a script can't call another script. This makes sharing and reusing code between scripts nearly impossible, short of client-side preprocessing (see the first point).
To address these needs while avoiding breaking changes to already-established and well-liked ephemeral scripts, Redis v7.0 introduces Redis Functions.

Use lua script with eval may encounter some scripts lost problem, and use function will be better. But we use the aliyun redis is v5, so we can not use it for now.

Strict order is not necessary for the task queue, it's ok to impl queue with two step.

@killagu killagu merged commit c2b7d5a into main Aug 4, 2022
@killagu killagu deleted the feat/sort_set_redis_queue branch August 4, 2022 11:21
@fengmk2
Copy link
Member

fengmk2 commented Aug 4, 2022

1.10.0

@Zheaoli
Copy link
Contributor

Zheaoli commented Aug 4, 2022

According to the redis doc.

Prior versions of Redis made scripting available only via the EVAL command, which allows a Lua script to be sent for execution by the server. The core use cases for Eval Scripts is executing part of your application logic inside Redis, efficiently and atomically. Such script can perform conditional updates across multiple keys, possibly combining several different data types.

Using EVAL requires that the application sends the entire script for execution every time. Because this results in network and script compilation overheads, Redis provides an optimization in the form of the EVALSHA command. By first calling SCRIPT LOAD to obtain the script's SHA1, the application can invoke it repeatedly afterward with its digest alone.

By design, Redis only caches the loaded scripts. That means that the script cache can become lost at any time, such as after calling SCRIPT FLUSH, after restarting the server, or when failing over to a replica. The application is responsible for reloading scripts during runtime if any are missing. The underlying assumption is that scripts are a part of the application and not maintained by the Redis server.

This approach suits many light-weight scripting use cases, but introduces several difficulties once an application becomes complex and relies more heavily on scripting, namely:

All client application instances must maintain a copy of all scripts. That means having some mechanism that applies script updates to all of the application's instances.

Calling cached scripts within the context of a transaction increases the probability of the transaction failing because of a missing script. Being more likely to fail makes using cached scripts as building blocks of workflows less attractive.

SHA1 digests are meaningless, making debugging the system extremely hard (e.g., in a MONITOR session).

When used naively, EVAL promotes an anti-pattern in which scripts the client application renders verbatim scripts instead of responsibly using the KEYS and ARGV Lua APIs.

Because they are ephemeral, a script can't call another script. This makes sharing and reusing code between scripts nearly impossible, short of client-side preprocessing (see the first point).

To address these needs while avoiding breaking changes to already-established and well-liked ephemeral scripts, Redis v7.0 introduces Redis Functions.

Use lua script with eval may encounter some scripts lost problem, and use function will be better. But we use the aliyun redis is v5, so we can not use it for now.

Strict order is not necessary for the task queue, it's ok to impl queue with two step.

Actually, the script-losing problem because the client use the cached script. BTW, this PR is LGTM for me if we can assume that we don't need atomic action here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants