Get rid of broadway #149
Get rid of broadway #149
Conversation
|
Hey, I can provide some context around this. One of our original goals for building this library was to provide a way to reliably push a job or batch of jobs to Faktory without the caller having to be concerned about the outcome. We took a fire and forget approach and allowed another part of the library to deal with any failures. Broadway allowed us to do this very quickly because it already had a mechanism for dealing with and retrying failed push attempts. We also deal with large spikes in our app that have in the past required us to schedule tens/hundreds of thousands of jobs in batch from a single run of a cron job. Without the above feature, the cron job would have to deal with parallelising the push to Faktory, then tracking and retrying any potential failures. However, Faktory now has support for batching jobs that might solve the same issue without the need for a custom implementation (I haven't looked at the feature in-depth). In which case simplifying the library is always a win 😃 |
|
@swelham thank you for explanation. I thought the same. Since Faktory has batch operations and there is module |
|
Yeah, it would be a disadvantage to some and a breaking change. However, I think for maintainability and parity with Faktory this tradeoff makes sense in the context of this library. After all, this only existed to plug a gap in Faktory where a desired feature previously didn't exist. In addition, any users of this library that are using the OSS version of Faktory could implement their own solution (with or without Broadway). Maybe a blog post detailing the original implementation would be useful, this could even be linked from the changelog to help any users migrate. |
Our plan is to more closely mirror the official client libraries. We'd be doing a major version bump and we'll write an upgrade guide.The right solution might be for people to implement their own batching if they're using OSS Faktory. |
|
@swelham are you folks still using this or have you fully transitioned off? |
lib/faktory_worker/job.ex
Outdated
| |> format_pipeline_name() | ||
| end | ||
|
|
||
| defp format_pipeline_name(name) when is_atom(name) do |
There was a problem hiding this comment.
Is this still needed? Looks like the pipeline name may be able to be completely eliminated?
| # But Faktory is a job server and do we want to care and build one more not needed layer??? | ||
| opts | ||
| |> faktory_name() | ||
| |> push(payload) |
There was a problem hiding this comment.
Switching from Broadway.push_messages to push is going to change the return of this function. From the PR discussion, I gather we are planning on doing a version change.
- Are there additional changes needed to this code base to signal that change?
- Does module/package documentation need to be updated?
There was a problem hiding this comment.
Not for this PR, just documenting a thought - but if we can return a Job with the faktory job id here, that would be a great gateway into being able to check on the status of a job.
There was a problem hiding this comment.
Broadway.push_messages and Sandbox.enqueue_job both always return :ok, so I think it should be OK to have this return {:ok, job_id} instead.
+1 for updating documentation as well
There was a problem hiding this comment.
it looks like we don't need perform_async/3 at all and we can return same result for any call. private function handle_push_result prepares result and it returns {:ok, job}.
I am not sure is it better to change to {:ok, job_id} or leave it as is. job contains a bit more info, see sample below:
{:ok,
%{
args: [%{hey: "there!"}],
jid: "cc7cb6ec723b1e1f35815ba1",
jobtype: "FaktoryWorker.DefaultWorker"
}}
There was a problem hiding this comment.
It should be OK to return it as-is. Can you also update Sandbox.enqueue_job to return a similar value? You can just generate a random jid.
There was a problem hiding this comment.
@superhawk610 we have changed return to {:ok, job}, but I checked our app and I see it always expect :ok.
Question is -> revert changes and return :ok as before, change app and it's tests to have support of {:ok, job). or 3rd option: add configuration option and return optionally :ok to keep back compatibilities and return by default {:ok, job}.
There was a problem hiding this comment.
Let's change our app to accept {:ok, job}
There was a problem hiding this comment.
Make sure that this is mentioned in the upgrade guide as well, as it's a breaking change.
|
This change means that For this PR, I think a couple more things are required:
|
@Ch4s3 We are no longer using Faktory and have completely transitioned off. |
| def perform_async(_, {:error, _} = error, _), do: error | ||
|
|
||
| def perform_async(pipeline_name, payload, opts) do | ||
| if Sandbox.active?() do |
There was a problem hiding this comment.
I think we want to keep the sandbox, it is what we use to simulate the server during testing.
There was a problem hiding this comment.
@jeremyowensboggs makes sense, I overdid it with simplifications
…terprise tag for specific tests
|
I've added couple of fixes:
|
|
It would be a good idea to add the bit about the tags and the enterprise tag to the README.md. |
removed broadway from dependencies.
It looks like PushPipeline isn't required at all.
It adds complexity with a Broadway (GenStage underneath) only for pushing jobs to Faktory.
I am not sure if I understood everything correctly but it seems we can just push job to Faktory. If assumption is correct I can clean-up code at job.ex ->perform_async function.