Background Job Manager (BJM) - replacement for BIO#356
Conversation
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #356 +/- ##
============================================
- Coverage 68.39% 68.11% -0.28%
============================================
Files 108 110 +2
Lines 61562 61769 +207
============================================
- Hits 42107 42076 -31
- Misses 19455 19693 +238
🚀 New features to boost your workflow:
|
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
madolson
left a comment
There was a problem hiding this comment.
Sorry, I've been ignoring this for ever but I think it's worth pulling in. Some inconsistencies in style and some odd name choices, but I'm happy to sit down and chat to get this merged.
|
|
||
| info = sdscatprintf(info, | ||
| "# BackgroundJobManager\r\n" | ||
| "bjm_num_threads:%d\r\n" |
There was a problem hiding this comment.
| "bjm_num_threads:%d\r\n" | |
| "background_job_num_threads:%d\r\n" |
I'm going to bring up that BJM means something else, and would prefer we not abbreviate it. I personally never liked the previous RIO or BIO abbreviations, because it was never clear what they meant.
| /* Initialize BJM with the requested number of background threads. | ||
| */ |
There was a problem hiding this comment.
Move the documentation to the .c files to better follow coding conventions.
| // Returns a new Fifo, containing all of the items from "q". "q" remains valid, but becomes empty. | ||
| // This is an O(1) operation. |
There was a problem hiding this comment.
By coding convention multi line comments need to do /* */
|
|
||
| // Returns a new Fifo, containing all of the items from "q". "q" remains valid, but becomes empty. | ||
| // This is an O(1) operation. | ||
| Fifo * fifoPopAll(Fifo *q); |
There was a problem hiding this comment.
| Fifo * fifoPopAll(Fifo *q); | |
| Fifo *fifoPopAll(Fifo *q); |
|
|
||
|
|
||
| // Look at the first item in the queue (without removing it). | ||
| // NOTE: asserts if the queue is empty. |
There was a problem hiding this comment.
I still think this is a weird and counter intuitive assumption. Both that the object can't be NULL and that is asserts on length.
There was a problem hiding this comment.
Would prefer to not use an acronym and just spell out bjm.
|
@JimB123 What do you think about getting this up to speed? Are you still interested in updating it? |
The BIO module is a source of minor annoyance. Background Job Manager (BJM) is designed as a modular replacement for BIO. Assuming there is positive response for this PR, the PR will be extended to convert existing BIO usage to BJM and remove BIO.
Motivations:
PR Contents:
Originally proposed to Redis and stalled here: redis/redis#12029
Note especially that in the original proposal, there was some concern about WAITAOF which currently relies on BIO to serialize 2 independent background jobs. There were 3 possible solutions mentioned: