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

Prefilter polled queues #242

Merged
merged 3 commits into from Dec 5, 2022
Merged

Prefilter polled queues #242

merged 3 commits into from Dec 5, 2022

Conversation

neob91-close
Copy link
Contributor

@neob91-close neob91-close commented Nov 30, 2022

This is a PoC for reducing the Redis load caused by each worker frequently polling the entire set of subqueues.

SSCAN lets us prefilter the queues we pull out of Redis, and seems pretty fast too:

production In [49]: r.scard("t:scheduled")
production Out[49]: 3919

production In [50]: timeit.timeit(lambda: r.smembers("t:scheduled"), number=5)
production Out[50]: 0.06490249998751096

production In [51]: timeit.timeit(lambda: r.sscan("t:scheduled", match="zoom.*", count=10000), number=5)
production Out[51]: 0.01752800800022669

@neob91-close neob91-close force-pushed the prefilter-polled-queues branch 3 times, most recently from 199cc42 to 6596f01 Compare November 30, 2022 15:56
Copy link
Contributor

@vtclose vtclose left a comment

Choose a reason for hiding this comment

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

A couple nitpicks but I'm not very knowledgeable with TT internals to comment on correctness.

tasktiger/worker.py Outdated Show resolved Hide resolved
tasktiger/worker.py Outdated Show resolved Hide resolved
@vtclose vtclose removed the request for review from tsx December 1, 2022 10:18
@neob91-close neob91-close marked this pull request as ready for review December 1, 2022 11:29
Copy link
Member

@thomasst thomasst left a comment

Choose a reason for hiding this comment

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

Looks good except the comment.

@@ -23,6 +24,7 @@
from .timeouts import JobTimeoutException

LOCK_REDIS_KEY = "qslock"
REDIS_GLOB_CHARACTER_PATTERN = re.compile(r"([?*\[\]])")
Copy link
Member

Choose a reason for hiding this comment

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

The relevant characters to be escaped are *, ?, [, and \ (https://github.com/redis/redis/blob/155acef51ac7826ed294a4b61d891f1c7a9a40ac/src/util.c#L58). You additionally have ] (which is fine but irrelevant), but you should also escape \, so this should be re.compile(r"([?*\[\]\\])") (or re.compile(r"([?*\[\\])")).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how I overlooked the \ character, thanks!

I am aware that it is not necessary to escape ], but to my knowledge Redis docs do not mention that fact and I prefer to avoid depending on undocumented behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Which docs btw? I was trying to find Redis docs on globs but couldn't find any, so then I just looked at the source code.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has some examples https://redis.io/commands/keys/ but doesn't seem like a formal spec

@neob91-close neob91-close merged commit f5acc2e into master Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants