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

Filtering in MUTATE API #489

Open
rustworthy opened this issue Nov 22, 2024 · 2 comments
Open

Filtering in MUTATE API #489

rustworthy opened this issue Nov 22, 2024 · 2 comments

Comments

@rustworthy
Copy link
Contributor

Hi there!

We are currently adding support for MUTATE API in Rust bindings.

Looks like one of the examples in the docs will not hold, namely:

MUTATE {"cmd":"kill","target":"retries","filter":{"jobtype":"QuickbooksSyncJob", "jids":["123456789", "abcdefgh"]}}

From the matchForFilter in the server code, we know that if they've provided jobtype there is no way they can reach the jids bit. Effectively, they can filter by jobtype + regexp OR only jobtype OR only jids. This can lead to the situation where they only want to apply MUTATE action to some specific (with jids=[..., ..., ]) jobs of certain type, but what will happen is all the jobs of this type will be affected. We've tried to demonstrate this in this test.

The docs will warn against using mutate actions as part of application logic, but - since they are part of our public API - I am wondering whether we should document that bit in the bindings as it is implemented now or any changes were planned but not implemented on the server side?

Please let me know if any further actions / details are needed from our side.

Regards,
Pavel

cc @jonhoo

@mperham
Copy link
Collaborator

mperham commented Nov 22, 2024

It doesn't make any sense to use jobtype + jid. Jid is a unique identifier; I'd think you should discard the jobtype.

I'd argue that the bug is that the Jid check on line 100 should be before line 83.

@rustworthy
Copy link
Contributor Author

It doesn't make any sense to use jobtype + jid. Jid is a unique identifier; I'd think you should discard the jobtype.

I'd argue that the bug is that the Jid check on line 100 should be before line 83.

One may think it does make sense if internally those jobs were grouped by jobtype within each target structure, in which case specifying a jobtype could really help narrow down the match candidates.

But I see, it's either jids OR optional jobtype + optional regex. I'll just document it and also add a warning that one should not use this api as part of application logic, rather only for administration needs.

Btw, having this api turned out to be is extremely helpful for testing: one can force-queue the retries or, say, scheduled jobs before each test, then purge the queue used for that particular test and start the test run a-fresh, which means we clean up things properly and the container with Faktory can be re-used. Also being able to "accelerate" retries for testing purposes is really handy.

Thank you!

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

No branches or pull requests

2 participants