Search #2190
Search #2190
Conversation
| const trace = currentJob['stacktrace']; | ||
| if (!Array.isArray(trace)) { | ||
| if (typeof trace === 'string') { | ||
| currentJob['stacktrace'] = JSON.parse(trace); |
There was a problem hiding this comment.
curious about this logic, https://github.com/taskforcesh/bullmq/blob/master/src/classes/job.ts#L316 fromJSON method is handling stacktrace values
There was a problem hiding this comment.
Definitely be better if we could just use Job.fromJson here.
| const raw = currentJob as JobJsonRaw; | ||
| const job = Job.fromJSON(queue, raw, jobId); | ||
| const ts = currentJob['timestamp']; | ||
| job.timestamp = ts ? parseInt(ts) : Date.now(); |
There was a problem hiding this comment.
| return type(val) == 'table' | ||
| end | ||
|
|
||
| local function isArray(t) |
There was a problem hiding this comment.
we can use includes to split this command as it's kind of big
There was a problem hiding this comment.
Agreed. I also intend to split out state list iteration methods using lua's pairs prrotocol.
There was a problem hiding this comment.
Impressive job overall. I could not review in detail all the .lua code, I will try to spend more time analyzing it as the PR is more ready.
A couple of remarks:
- This script can potentially put a lot of pressure on Redis, as it will be CPU intensive. I think it should be wise to try to move as much logic as possible to the JS side. Maybe preprocess the query so that the job to do on lua side would be minimal. A crazy extreme of this would actually be to generate the lua code in JS that actually is used to check every job and then use something like this: https://www.lua.org/manual/5.1/manual.html#pdf-loadstring
- The method must allow to limit for amount of returned jobs per call, and also the amount of jobs to test for. As without any of these two constraints the call to this script could potentially hang the Redis server for too long. More than 100ms per call will probably be too slow in practice.
| @@ -355,6 +355,15 @@ export class QueueGetters< | |||
| ); | |||
| } | |||
|
|
|||
| async getJobsByFilter( | |||
There was a problem hiding this comment.
Don't forget the typedoc style documentation for this public method.
| const trace = currentJob['stacktrace']; | ||
| if (!Array.isArray(trace)) { | ||
| if (typeof trace === 'string') { | ||
| currentJob['stacktrace'] = JSON.parse(trace); |
There was a problem hiding this comment.
Definitely be better if we could just use Job.fromJson here.
| ['id'] = 1, | ||
| } | ||
|
|
||
| local JsType = { |
There was a problem hiding this comment.
I think that JsType should use the same name convention as the rest of the constants. Or the rest of the constant the same convention as JsType 😅
| } | ||
| } | ||
|
|
||
| for (let i = 1; i < response.length; i += 2) { |
There was a problem hiding this comment.
Ok, so the thing that makes this code more complex seems to be that all the jobs are returned flattened in one single array. Wouldn't be better to return an array of arrays from the lua script so that we could have much simpler code here?
| for _, jobId in pairs(scannedJobIds) do | ||
|
|
||
| local jobIdKey = keyPrefix .. jobId | ||
| local jobHash = redis.pcall('HGETALL', jobIdKey) |
There was a problem hiding this comment.
Maybe we could just restrict the search to the "data" field.
There was a problem hiding this comment.
Even though we later need all the fields of the job, if we assume the search would just match a narrow set of jobs, it would be more efficient to read the remaining fields only for the jobs that actually match the search.
There was a problem hiding this comment.
Good catch. The code actually collects the fields that are accessed in the query so that's possible with not too much problem.
|
Any update ? |
|
@OrionWambert Ping me. I may make some time over the weekend to pick this back up. |
Adds job search functionality using
mongosyntax.