Conversation
Signed-off-by: Balazs Scheidler <[email protected]>
sodomelle
force-pushed
the
scalable-disk-queue
branch
from
e4b9822 to
d788bd8
Compare
|
I'd like to review this too. |
alltilla
left a comment
alltilla
left a comment
There was a problem hiding this comment.
I am done with one round of review, I will do 1-2 more rounds.
|
As I've heard the code builds on |
|
Done with another round of review, I won't go another one until we process my comments so far. |
sodomelle
force-pushed
the
scalable-disk-queue
branch
2 times, most recently
from
6369a34 to
27b5e31
Compare
|
Current implementation involves the possibility of requiring more memory due to |
alltilla
left a comment
alltilla
left a comment
There was a problem hiding this comment.
As we talked IRL, I had some coding nitpicks, and came to the conclusion that it would be best if I "fixed" them and opened a PR for you. I don't insist on merging the code organization changes, but kindly consider them, I think they make the code easier to read and maintain a bit.
|
Thank you for the fixes! |
Signed-off-by: Balazs Scheidler <[email protected]>
This shifts the diskq implementation to be more similar to LogQueueFifo as a preparation for taking over similar scalability fixes from LogQueueFifo. Signed-off-by: Balazs Scheidler <[email protected]>
…mory queues Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
To avoid frequently locking, when popping messages from the queues, we introduce a new queue, that takes messages in batches. This new queue requires no locking afterwards. The new queue has the same size as the front_cache, and requires front_cache to be configured to work. Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
…queue Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
In order to find the first element, we should check the front_cache_output queue first. If it doesn't contain any messages we peek the lock protected queues. Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
In case of rewind, messages from the backlog should go to front_cache_output instead of front_cache. Backlog and front_cache_output queues don't require locking. Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
Signed-off-by: shifter <[email protected]> Signed-off-by: Tamás Kosztyu <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
A bit easier for me to read the important functions like pop_head. Signed-off-by: Attila Szakacs <[email protected]>
…nts() Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
* Eliminates one lock() call * Flattens the if statements * Improves intention behind goto statements Signed-off-by: Attila Szakacs <[email protected]>
Signed-off-by: Attila Szakacs <[email protected]>
sodomelle
force-pushed
the
scalable-disk-queue
branch
from
c4aaf39 to
e5feb09
Compare
Signed-off-by: Attila Szakacs <[email protected]>
sodomelle
force-pushed
the
scalable-disk-queue
branch
from
e5feb09 to
0269492
Compare
MrAnno
left a comment
MrAnno
left a comment
There was a problem hiding this comment.
I can't do a thorough review within 1 day. Even with two more review rounds, I would still suggest writing new unit tests and Light tests as well to validate the different scenarios (especially when queues gets full and empty + their persistence when shutting down).
I think we can merge the PR today if we agree to wait a week with the release and we write more tests.
|
|
||
| // Diskbuffer is the part of disk-queue that is used only when front cache is full | ||
| Test(diskq_truncate, test_diskq_truncate_with_diskbuffer_used, .disabled=true) | ||
| Test(diskq_truncate, test_diskq_truncate_with_diskbuffer_used) |
There was a problem hiding this comment.
We identified this test as incorrect, because it didn't even truncate the buffer (it was incorrect originally, long before this PR). We should rewrite this test so that it does what it was meant for.
There was a problem hiding this comment.
Anyways, simply disabling it in an "update tests" commit without any note is not so good.
4 participants
@bazsi did a bunch of changes. The most important one in our case is that he changed from
g_liststoivykis listfor in-memory queues inLogQueueNonReliableDisk.@bshifter and I added a new output queue, that takes batches of messages instead of a single message, to avoid frequent locking and unlocking.