fix: extendlock,createbulk use pipeline no multi command #2584
Conversation
Signed-off-by: adi_holden <adi@dragonflydb.io>
| @@ -256,18 +256,18 @@ export class Job< | |||
| new this<T, R, N>(queue, job.name, job.data, job.opts, job.opts?.jobId), | |||
| ); | |||
|
|
|||
| const multi = client.multi(); | |||
There was a problem hiding this comment.
There is a small issue with this change, and it will be that the order of the jobs will not be preserved, I think that for most users this will not matter at all, but it could break it for some users. I think we should add an new option to BulkJobOptions where the user can choose between using pipeline or multi, and default to multi.
There was a problem hiding this comment.
The order of jobs inside a specific bulk will be preserved, what could be affected is if there is more than one calls to add bulk at the same time than now the jobs of different bulks can change as we can add job from bulk1 and then job from bulk2 and than another job from bulk1. Is this your concern?
I think that if we add this option to BulkJobOptions and will not be the default no one will use it actually.
There was a problem hiding this comment.
I was thinking that it could potentially happen that that the order is not preserved for one bulk. If this is not the case then I think we are fine in merging this as is.
| @@ -1004,16 +1004,16 @@ will never work with more accuracy than 1ms. */ | |||
|
|
|||
| protected async extendLocks(jobs: Job[]) { | |||
| try { | |||
| const multi = (await this.client).multi(); | |||
| const pipeline = (await this.client).pipeline(); | |||
There was a problem hiding this comment.
Here the order does not matter at all, so it is fine to just replace to pipeline.
client.multi() creates a transaction of multiple commands (all commands to exec call), to ensure atomicity of multiple commands.
As extendlock and createbulk do not require atomicity for multiple commands pipeline can be used to reduce the round trip.