Mutate API #87

rustworthy · 2024-11-19T08:09:01Z

addresses #61

this should also "unblock" #88

This change is

codecov · 2024-11-20T07:02:59Z

Codecov Report

Attention: Patch coverage is 82.63158% with 33 lines in your changes missing coverage. Please review.

Project coverage is 68.7%. Comparing base (6ff7d60) to head (29f0259).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/proto/client/mutation.rs	62.6%	31 Missing ⚠️
src/worker/mod.rs	81.8%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/proto/client/mod.rs	`90.0% <ø> (+2.9%)`	⬆️
src/proto/mod.rs	`72.7% <ø> (ø)`
src/proto/single/cmd.rs	`97.9% <100.0%> (+0.4%)`	⬆️
src/proto/single/mod.rs	`99.2% <100.0%> (+5.0%)`	⬆️
src/proto/single/mutation.rs	`100.0% <100.0%> (ø)`
src/proto/single/utils.rs	`99.1% <100.0%> (+<0.1%)`	⬆️
src/worker/mod.rs	`84.0% <81.8%> (-1.1%)`	⬇️
src/proto/client/mutation.rs	`62.6% <62.6%> (ø)`

... and 2 files with indirect coverage changes

jonhoo

Haven't gotten all the way through this, but racked up a few comments so figured I'd leave them with you!

jonhoo · 2024-12-31T10:28:42Z

src/lib.rs

 pub use crate::proto::{
-    Client, Connection, DataSnapshot, FaktoryState, Job, JobBuilder, JobId, Reconnect,
-    ServerSnapshot, WorkerId,
+    Client, Connection, DataSnapshot, FaktoryState, Job, JobBuilder, JobId, MutationFilter,
+    MutationFilterBuilder, MutationTarget, Reconnect, ServerSnapshot, WorkerId,
 };


This is starting to be a lot of top-level exports. How do you feel about starting to group some of them the way we've done with the pub mod ent below? Like pub mod mutate for example? Then we can also rename them to avoid the Mutation name prefix.

added namespace and - indeed - prefix no longer needed in the public api

though after some refactoring there are only two members left in that namespace :)

jonhoo · 2024-12-31T10:30:34Z

src/proto/single/cmd.rs

-        Ok(w.write_all(b"INFO\r\n").await?)
-    }
-}
+self_to_cmd!(Info);


Nice, I like the macro expansion here.

jonhoo · 2024-12-31T10:32:26Z

src/proto/single/cmd.rs

+pub(crate) struct MutationAction<'a> {
+    pub(crate) cmd: MutationType,
+    pub(crate) target: MutationTarget,
+    #[serde(skip_serializing_if = "filter_is_empty")]


Is an empty-but-set mutation filter actually == to an unset filter? In some systems, the two have different semantics (usually, unset means "use the default").

A nil filter and an empty one are treated in the common manner - as if we were matching everything in the targeted queue (they are wildcards effectively). Added a comment.

I see what you mean, like what if the default behavior changes? well, as one put it, where possible, your interface should be intuitive enough that if the user has to guess, they usually guess correctly. Looking at the filter fields (jids, regexp, jobtypes) I am guessing that leaving those empty should be a considered a wildcard or should cause an error encouraging me to be specific. In the same manner, omitting the filter - I am guessing - should be either a wildcard or (yet again) should error back.

P.S.: to give an example of a broken law of least astonishment: in case of an unset filter, we system would be targeting the jobs whose jobtype is a string literal "general" 😢

jonhoo · 2024-12-31T10:36:19Z

src/proto/client/mutation.rs

+    /// This method will immediately move the jobs from the targeted set (see [`MutationTarget`])
+    /// to their queues. This will apply to the jobs satisfying the [`filter`](crate::MutationFilter).


I'm having a hard time following what this operation actually does. For example, with MutationTarget::Scheduled, what does it mean for the jobs to "immediately be moved to their queues"? They're already in queues?

The scheduled set is not a queue, it’s time-sorted data structure in Redis. The job is moved from that structure into its queue for a worker to fetch.

Actually they are not in queues, rather in dedicated sets where they are "pending" basically.
Updated the docs and also added an example scenario

P.S. Left the draft reply open in the browser tab and did not see Mike's comment when publishing. "Multiversion concurrency control" made my comment look stupid 😓

jonhoo · 2024-12-31T10:36:47Z

src/proto/single/mutation.rs

+#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize)]
+#[serde(rename_all = "lowercase")]
+#[non_exhaustive]
+pub enum MutationTarget {


Is this actually specific to mutations? Feels like a general purpose job set specifier to me.

It's also weird to me that the "mutation target" isn't just another filter, given that seems to be the function it has.

the job sets are only used in mutation API context and also declared in the mutate module in the Go bindings. As per another thread, we decided to remove the Mutation prefix from the constucts in the mutation namespace, so it's now simply Target. But I see what you mean this could be in theory simply JobSet or JobSetKind.

well, this where I was hesitant for a while, but decided to follow the Go bindings way. open to revisiting this place though

jonhoo · 2024-12-31T10:40:21Z

src/proto/client/mutation.rs

+    /// client.requeue(MutationTarget::Retries, &filter).await.unwrap();
+    /// # });
+    /// ```
+    pub async fn requeue<'a, F>(&mut self, target: MutationTarget, filter: F) -> Result<(), Error>


The argument name target makes me think that the jobs will be moved to this, but I don't think that's the case. Isn't it more like source or candidate_job_set or something?

At the time of writing I was in the Mutate API flow and my idea was that we were mutating those dedicated sets and they were the mutation target, but when I now revisit this bit I see that it's confusing indeed.

I love source as parameter name, but then I will need to rename the Target to JobSet, otherwise we will get source: Target 😃 in the signature.

On the second thought, candidate_job_set looks more universal since we also have such methods as Client::discard and Client::kill and Client::clear

src/proto/single/mutation.rs

jonhoo · 2024-12-31T10:41:23Z

src/proto/single/mutation.rs

+    pub kind: Option<&'a str>,
+
+    /// Ids of jobs to target.
+    #[serde(skip_serializing_if = "Option::is_none")]


Should it be send if empty?

No need to. Even though is does not change the outcome, we should be consistent. So I added a utility trait called Empty (only for usage within the crate) so that we can do:

#[serde(skip_serializing_if = "Empty::is_empty")]

Also added a few unit tests for Filter and MutationAction for visualization purposes solely

src/proto/client/mutation.rs

jonhoo · 2024-12-31T10:42:47Z

src/proto/single/mutation.rs

+/// As of Faktory version 1.9.2, if [`MutationFilter::pattern`] and/or [`MutationFilter::kind`]
+/// is specified, the values in [`MutationFilter::jids`] will not be taken into account by the
+/// server. If you want to filter by `jids`, make sure to leave other fields of the filter empty
+/// or use dedicated methods like [`Client::requeue_by_ids`].


If that's the case, I wonder if we shouldn't derive builder and instead just have specific constructors (for now) for the combinations that do work. What do you think?

I think, this is the right call. If I do not read the docs (which I definitely can) and specify both kind and jids on the builder (thinking I am helping the server find jobs in question faster), I am getting into a situation where all the jobs of that kind in the targeted set are affected. If the server ignored pattern and kind when jids are there (which is a trivial fix as Mike points out here, there would be no harm actually.

So I think the constructors will be:

Filter::by_ids; Filter::by_kind; Filter::by_pattern; Filter::by_kind_and_pattern;

And the fields on that struct should be made private apparently.

went with Filter::from_<attr> rather than Filter::by_<attr>. the latter is better for a verb I think, like Client::filter_by_ids.

Co-authored-by: Jon Gjengset <[email protected]>

* Make Failure public * Update changelog * Get more info from panicking * Check different ways a job can fail * Add test clean up * Account for re-queued jobs * Check job id * Collect all kinds of handler failure * Add Job::failure_message. Check all error messages * Add to CHANGELOG.md. Fix clippy warning * Update tests/real/community.rs Co-authored-by: Jon Gjengset <[email protected]> * Update src/proto/single/mod.rs Co-authored-by: Jon Gjengset <[email protected]> * Fix docs to Failure * Nuke Job::failure_message * Nuke Job::failure_message * Restore comment in test_panic_and_errors_in_handler test * Add to the tests docs * Do not make Failure::kind pub --------- Co-authored-by: Jon Gjengset <[email protected]>

rustworthy · 2025-02-28T19:31:07Z

Haven't gotten all the way through this, but racked up a few comments so figured I'd leave them with you!

Sorry it took me a while to iterate on this PR - got some things to do in the wewerewondering project and also was moving countries with my partner and cats (since we implement Send 😄 )

Please have a look at what we got.

rustworthy and others added 3 commits November 19, 2024 12:08

#61 Add test

5a96b3a

Improve self_to_cmd macro

eec5bba

Add MutationAction to commands

2cf61a8

rustworthy added 17 commits November 20, 2024 18:50

Locate new logic in dedicated module

f61d4c0

Update CHANGELOG.md

51bcb8a

Update crate version

ee3d90d

Make Makefile commands more ergonomic

72ee004

Add requeue method

1e8839a

Test Client::requeue on failed jobs

44334ba

Option::is_none_or not available in Rust 1.70

fce242b

Use owned patter for JobFilterBuilder

d3639f0

Add more docs and tests for Client::requeue

e24c960

Add better test isolation

48fb985

Add Client::kill, Client::dicard, Client::clear

ac9da93

Test Client::clear

85993d3

Fix minimal version job

54eecc3

Add docs and conveniences like Client::requeue_by_ids

a883f57

Add test for Client::kill plus Client::requeue

17cdb4c

Test Client::discard

22a4d18

Fix typos

cded093

rustworthy changed the title ~~[WIP] Mutate API~~ Mutate API Nov 23, 2024

rustworthy requested a review from jonhoo November 23, 2024 09:19

jonhoo requested changes Dec 31, 2024

View reviewed changes

rustworthy and others added 6 commits December 31, 2024 15:01

Update src/proto/client/mutation.rs

7d9a94c

Co-authored-by: Jon Gjengset <[email protected]>

Update src/proto/single/mutation.rs

8346f08

Co-authored-by: Jon Gjengset <[email protected]>

Group mutate consturct under mutate prefix

56f0a2a

Add comment to mutataion action

1e95551

Make reviewdog happy

688edd1

rustworthy added 6 commits February 27, 2025 10:31

Fix docs

f858e73

Update docs to Client::requeue

12ae62d

Rename Target to JobSet

5425802

Use candidate_job_set instead of target

59fc7db

Add Empty trait

ab6e2a9

Use predefined constructors over Filder::builder

29f0259

rustworthy requested a review from jonhoo February 28, 2025 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutate API #87

Mutate API #87

rustworthy commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

jonhoo left a comment

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

rustworthy Feb 28, 2025

jonhoo Dec 31, 2024

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

rustworthy Feb 27, 2025

jonhoo Dec 31, 2024

mperham Feb 27, 2025

rustworthy Feb 27, 2025 •

edited

Loading

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

rustworthy Feb 27, 2025

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

rustworthy Feb 27, 2025

jonhoo Dec 31, 2024

rustworthy Feb 27, 2025

rustworthy Feb 28, 2025

rustworthy commented Feb 28, 2025

		/// This method will immediately move the jobs from the targeted set (see [`MutationTarget`])
		/// to their queues. This will apply to the jobs satisfying the [`filter`](crate::MutationFilter).

Mutate API #87

Are you sure you want to change the base?

Mutate API #87

Conversation

rustworthy commented Nov 19, 2024 • edited Loading

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

jonhoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustworthy Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustworthy commented Feb 28, 2025

rustworthy commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

rustworthy Feb 27, 2025 •

edited

Loading