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

refactor: Replace machinery with tasqueue #32

Merged
merged 4 commits into from
Sep 21, 2023
Merged

refactor: Replace machinery with tasqueue #32

merged 4 commits into from
Sep 21, 2023

Conversation

kalbhor
Copy link
Contributor

@kalbhor kalbhor commented Sep 9, 2023

This MR aims to replace the underlying job library (machinery) with tasqueue. Apart from internal changes that do not affect this libraries' clients, the following changes/suggestions are made:

  • Manage the runtime cleanly via contexts. Currently I've passed a simple context to the Start() method. We can discuss how to handle the cancellation (this is omitted as of now).

  • Change queue/concurrency configs. Tasqueue expects the concurrency parameter to be passed when registering the task (with some certain queue), unlike the existing way where concurrency is being passed when a group job is supposed to be queued. (We can discuss how to make this cleaner for clients)

  • Log management: Tasqueue uses zerodha/logf internally, maybe we can use this across the project

  • Ordering of pending jobs: The current testcases assume that the pending jobs returned will be in the same order as submission. This is implemented in a reverse manner in Tasqueue

  • Change in config: Instead of using DSNs to pass configs, have added a declarative config for both the results and broker options for Tasqueue.

PS: Any readme related changes can be made once the MR is finalized

@knadh knadh added the enhancement New feature or request label Sep 10, 2023
@knadh
Copy link
Collaborator

knadh commented Sep 10, 2023

This is excellent. The Machinery build is 26MB. The Tasqueue build is 7.9MB 🎉

Hasn't the new stdlib slog superseded zerodha/logf? @mr-karan?

@mr-karan
Copy link
Member

Hasn't the new stdlib slog superseded zerodha/logf?

Yes, we plan to support slog backend in zerodha/logf eventually but the API will be slog itself. Better to use that here as we can switch backends later but have a consistent logging API.

@knadh
Copy link
Collaborator

knadh commented Sep 11, 2023

@kalbhor let's replace the zerodha/logf with slog in Tasqueue?

@kalbhor
Copy link
Contributor Author

kalbhor commented Sep 20, 2023

Have replaced zerodha/logf with slog in Tasqueue. It will accept an appropriate slog.Handler (which can be swapped later if needed, like mr-karan has mentioned). Made these changes in DungBeetle as well, as a part of this PR

cmd/config.sample.toml Outdated Show resolved Hide resolved
cmd/config.sample.toml Outdated Show resolved Hide resolved
@knadh knadh merged commit 09e09f0 into zerodha:rebrand Sep 21, 2023
knadh pushed a commit that referenced this pull request Mar 13, 2024
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.

3 participants