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

feature: support dumping jobs to disk. #11

Closed
wants to merge 1 commit into from

Conversation

sbalaji1996
Copy link

@sbalaji1996 sbalaji1996 commented Feb 1, 2020

Resolves: #2

@sbalaji1996 sbalaji1996 requested a review from ttacon February 1, 2020 01:25
* @param {Object} [filter={}] The sift-compatible filter
*/
async dump(jobType, filter = {}) {
const matches = await this._find(jobType, filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd make sense to add a forEach method so that we could write the jobs out without needing to have them all in memory.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah, makes sense

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, there could be a ton of jobs here - so paginating would be ideal. It might make sense to add a batch processor to handle matches as we find them here.

Copy link
Contributor

Choose a reason for hiding this comment

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

On that - #14

Copy link
Owner

@ttacon ttacon left a comment

Choose a reason for hiding this comment

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

Agreed with @shils , we need to handle the case where there could be a very large number of jobs. Otherwise, the rest of this PR LGTM.

@@ -7,6 +7,9 @@ const redis = require('redis');
const repl = require('repl');
const sift = require('sift').default;
const getValue = require('get-value');
const waitOn = require('promise-callbacks').waitOn;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const waitOn = require('promise-callbacks').waitOn;
const { waitOn } = require('promise-callbacks');

* @param {Object} [filter={}] The sift-compatible filter
*/
async dump(jobType, filter = {}) {
const matches = await this._find(jobType, filter);
Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, there could be a ton of jobs here - so paginating would be ideal. It might make sense to add a batch processor to handle matches as we find them here.

await waitOn(jsonWriteStream, 'finish', { waitError: true });
console.log(`Dumped ${cleanedMatches.length} jobs to ${filename}`);
} catch (err) {
console.error('Failed to write to fail', err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.error('Failed to write to fail', err);
console.error('Failed to write to file', err);

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.

Add support for searching jobs by filter and dumping them to a JSON file
3 participants