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

Enterprise batch support #139

Merged
merged 12 commits into from
Nov 3, 2021
Merged

Enterprise batch support #139

merged 12 commits into from
Nov 3, 2021

Conversation

Ch4s3
Copy link
Collaborator

@Ch4s3 Ch4s3 commented Oct 20, 2021

This PR adds support for the enterprise batching functionality.

The functionality is loosely based on the official libraries, but attempts to stick to the existing patterns here.

Note that we have excluded the batch tests from CI because they require an enterprise version of faktory to run. Perhaps we could later ask contribsys to provide a key for testing here.

killahwave and others added 12 commits November 13, 2020 14:42
fix names

correcting issues with incorrect use of bid

add connectionmanager to batches

remove warnings

telemetry

remove usage

fix protocol

fix telemetry

updates

updates per dav

updates based on feedback

adding default

default

update arguments

updates
Auto 1688 - FaktoryWorker Batch extension
fix names

correcting issues with incorrect use of bid

add connectionmanager to batches

remove warnings

telemetry

remove usage

fix protocol

fix telemetry

updates

updates per dav

updates based on feedback

adding default

default

update arguments

updates
This cleans up the batch support by:

1. Collapsing the `build_payload/2` and `new/2` commands into `new!/2`. This
   provides a better experience to users of the library.

2. Providing defaults for all `opts` for batch related functions. Otherwise
   callers are required to provide `[]` as the second argument for every
   command which can be tedious.

3. Providing `status/2` as a function and removing it from the test helpers.

4. Improving the tests.

5. Adding more documentation to functions with explanations of what arguments
   are required, what options are allowed, and what is returned.
This introduces the ability for jobs to be pushed synchronously using the option
`skip_pipeline: true`. This is necessary when pushing jobs as part of a batch to
ensure they are submitted prior to the batch being committed--without this,
there's the possibility that jobs for the batch are left on the PushPipeline
when the batch is committed, and those jobs are not included in the batch.

As part of this work, the logic for pushing jobs was refactored so that it lives
in the `FaktoryWorker.Job` module and `FaktoryWorker.PushPipeline.Consumer`
relies on the `FaktoryWorker.Job.push/2` function to push work.
@john-griffin
Copy link
Contributor

Hey thanks for the PR!

At Seated we are not using the batch feature from enterprise. In fact this comes at the point where we have almost completed the transition of our app from Faktory to Oban. We don't want to stand in the way here so are you guys at Pepsi interested in taking over the project seeing as we won't be actively maintaining the library any further?

@Ch4s3
Copy link
Collaborator Author

Ch4s3 commented Nov 1, 2021

Hey thanks for the PR!

At Seated we are not using the batch feature from enterprise. In fact this comes at the point where we have almost completed the transition of our app from Faktory to Oban. We don't want to stand in the way here so are you guys at Pepsi interested in taking over the project seeing as we won't be actively maintaining the library any further?

Let me look into it.

The easiest thing is probably to move it to a new org, and give some of us commit access.

@john-griffin
Copy link
Contributor

Thanks, works for me, just let me know!

@Ch4s3
Copy link
Collaborator Author

Ch4s3 commented Nov 1, 2021

Cool, let me think of a good org name and create one to transfer this too. That seems like the most expedient path.

@Ch4s3
Copy link
Collaborator Author

Ch4s3 commented Nov 2, 2021

@john-griffin you can transfer to https://github.com/opt-elixir and maybe add me in hex.pm. I'll rope some other folks in and make sure this gets taken care of.

@john-griffin
Copy link
Contributor

@Ch4s3 thanks. I've made you an owner on hex and made you an admin of this repo. You should be able to transfer it to opt-elixir now.

@Ch4s3
Copy link
Collaborator Author

Ch4s3 commented Nov 3, 2021

@john-griffin Thanks! If you folks ever move back to using this, let me know.

@Ch4s3 Ch4s3 merged commit b2d12de into opt-elixir:master Nov 3, 2021
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.

None yet

5 participants