using per-key basis queue #5420
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5420 +/- ##
==========================================
+ Coverage 93.93% 93.96% +0.02%
==========================================
Files 123 124 +1
Lines 9005 9047 +42
==========================================
+ Hits 8459 8501 +42
Misses 546 546
Continue to review full report at Codecov.
|
|
Nice work! I have two humble suggestions for your PR:
Maybe something like this: beforeOp(key) {
let obj = this.queues.get(key);
if (!obj) {
obj = [0, Promise.resolve()];
this.queues.set(key, obj);
}
obj[0]++;
}
afterOp(key, obj) {
let count = obj[0];
if (count === undefined) {
return;
}
count--;
if (count <= 0) {
this.queues.delete(key);
return;
}
obj[0] = count;
}
enqueue(key, operation) {
let obj = this.beforeOp(key);
const toAwait = obj[1];
const nextOperation = toAwait.then(operation);
const wrappedOperation = nextOperation.then(result => {
this.afterOp(key, obj);
return result;
});
obj[1] = wrappedOperation;
return wrappedOperation;
} |
|
@saulogt I have modified the code reflecting your suggestions, you're right using one object is cleaner and faster, let me know what you think. |
|
@georgesjamous It looks great |
|
Travis check has passed but for some reason, it's not being updated in the PR. Is there a way to restart the test without a commit? @acinader I don't use codecov much, can you be a little bit more specific? |
|
@georgesjamous This PR looks great. I've restarted the test for you. If you want to restart it just close and reopen the PR. Codecov shows which parts of your code haven't been tested. The goal is to have as much tested as possible to avoid regression. Use |
|
Alright, I have added additional tests and made some minor alterations. |
* adding KeyPromiseQueue * nit * removing secondary object and using a tuple * using array * nits * some tests * Minor refinements * removing old adapter * dummy change, travis test not found * travis test missing, dummy change * revrting mistake * reverting mistake * indentation fix * additional tests for coverage * extending coverage * nits * fixing mistake * better code
Currently, the RedisCacheAdapter uses a global promise queue chain to handle redis cache operations.
parse-server/src/Adapters/Cache/RedisCacheAdapter.js
Line 19 in f798a60
This produces a bottleneck point and causes several problems during high requests such as timeouts and delays since all cache operations are chained and depend on each other. As discussed in this ticket #5401
I believe that initially it was designed that way to allow maximum data consistency and prevent operations from overlapping.
A potential solution would be to chain the cache operations per key basis, that way consistency on-key-basis is maintained and that would allow parallel operations to be invoked, which would loosen up the single bottleneck point.
I have used a custom KeyPromiseQueue class as opposed to the built-in MemoryCache since ttl should not be used because expiring active operations would mean the next ones would not be chained (since the previous ones are not tracked anymore) and data consistency would be lost again. Which is our main concern here, otherwise we would not use any chain.
Any thoughts or concerns on such modification?