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

Batches and trackable jobs #48

Merged
merged 65 commits into from
Feb 18, 2024
Merged

Conversation

rustworthy
Copy link
Collaborator

@rustworthy rustworthy commented Jan 17, 2024

As per issue

  • Add support for batch jobs
  • Add tracker responsible for tracking jobs and batch statuses;

This change is Reviewable

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 187 lines in your changes are missing coverage. Please review.

Comparison is base (0bd2ab0) 74.9% compared to head (62ec997) 68.4%.

Additional details and impacted files
Files Coverage Δ
src/consumer/mod.rs 69.6% <100.0%> (+0.5%) ⬆️
src/proto/single/ent/mod.rs 100.0% <100.0%> (ø)
src/proto/single/utils.rs 100.0% <100.0%> (ø)
src/proto/single/mod.rs 93.9% <90.0%> (-0.5%) ⬇️
src/proto/single/ent/utils.rs 0.0% <0.0%> (ø)
src/producer/mod.rs 65.0% <26.6%> (-11.8%) ⬇️
src/proto/single/ent/cmd.rs 0.0% <0.0%> (ø)
src/proto/single/resp.rs 84.7% <0.0%> (-4.9%) ⬇️
src/proto/batch/cmd.rs 0.0% <0.0%> (ø)
src/proto/mod.rs 70.3% <43.5%> (-9.2%) ⬇️
... and 2 more

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Awesome stuff, thank you! Left a few notes, but none that I think are particularly onerous 🎉

src/consumer/mod.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/producer/mod.rs Outdated Show resolved Hide resolved
src/producer/mod.rs Outdated Show resolved Hide resolved
src/producer/mod.rs Outdated Show resolved Hide resolved
src/proto/single/ent.rs Outdated Show resolved Hide resolved
src/proto/single/utils.rs Outdated Show resolved Hide resolved
src/proto/single/mod.rs Outdated Show resolved Hide resolved
src/proto/single/resp.rs Show resolved Hide resolved
src/tracker/mod.rs Outdated Show resolved Hide resolved
@rustworthy rustworthy mentioned this pull request Jan 31, 2024
7 tasks
@rustworthy rustworthy requested a review from jonhoo February 12, 2024 19:13
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Basically just nits left!

One thought I have: is there even a reason to have Producer and Client be different types? Like, could we literally change Producer to

pub type Producer<S> = Client<S>;

?

src/proto/batch/mod.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
src/proto/mod.rs Show resolved Hide resolved
src/proto/mod.rs Show resolved Hide resolved
src/proto/mod.rs Outdated Show resolved Hide resolved
rustworthy and others added 8 commits February 17, 2024 21:04
@rustworthy
Copy link
Collaborator Author

rustworthy commented Feb 17, 2024

Basically just nits left!

One thought I have: is there even a reason to have Producer and Client be different types? Like, could we literally change Producer to

pub type Producer<S> = Client<S>;

?

Hmm we could not without exposing methods enqueue , info, queue_pause, queue_resume, start_batch, open_batch and commit_batch on the Client.

@rustworthy rustworthy requested a review from jonhoo February 17, 2024 17:43
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Going to land this as that discussion is a bit separate, and would probably be breaking.

@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2024

Hmm we could not without exposing methods enqueue , info, queue_pause, queue_resume, start_batch, open_batch and commit_batch on the Client.

Right, but I guess my argument is that there isn't semantically really a difference between a Client and a Producer (I think?).

@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2024

Looks like #54 caused some conflicts with this one — I've resolved them and pushed directly to your branch to merge 👍

@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2024

Worth highlighting the change I made in 6c98127 too. I also decided to do that because it meant my editor kept changing the uses, and then the job requires that I run a manual command to change them back, which became tedious pretty quickly 😅

@jonhoo jonhoo merged commit cdf15d3 into jonhoo:main Feb 18, 2024
16 of 18 checks passed
@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2024

Released in 0.12.5 🎉

@rustworthy
Copy link
Collaborator Author

Hmm we could not without exposing methods enqueue , info, queue_pause, queue_resume, start_batch, open_batch and commit_batch on the Client.

Right, but I guess my argument is that there isn't semantically really a difference between a Client and a Producer (I think?).

I guess semantically both Producer and Consumer are Clients talking to the Factory Server. The fact that Go bindings only use Client speaks for itself. At the same time - just a refresher - the Faktory lingo has got a Client Vs Worker. So, if we decide to merge client with producer, we may want to rename the Consumer to be Worker, wdyt?

I believe, though, the current setup IS semantically correct.

@jonhoo
Copy link
Owner

jonhoo commented Feb 18, 2024

Yeah, that's my thinking too: Producer -> Client, Consumer -> Worker, and have tracking just be additional methods on Client. After all, there's not really any difference between a client that submits jobs and one that tracks them.

@rustworthy
Copy link
Collaborator Author

Worth highlighting the change I made in 6c98127 too. I also decided to do that because it meant my editor kept changing the uses, and then the job requires that I run a manual command to change them back, which became tedious pretty quickly 😅

Hmm the idea was to never have to return to the question of 'use's grouping :), where the pipeline job would be the guard, rather than a reviewer. Anyhow, it was 1) a nit and 2) a nice nightly feature to learn about 🔬

@rustworthy
Copy link
Collaborator Author

Yeah, that's my thinking too: Producer -> Client, Consumer -> Worker, and have tracking just be additional methods on Client. After all, there's not really any difference between a client that submits jobs and one that tracks them.

Will be addressed in #49

@rustworthy rustworthy deleted the feat/ent-batch-jobs branch February 21, 2024 14:43
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.

2 participants