-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add KeyPromiseQueue to Push and Job StatusHandlers #7267
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7267 +/- ##
==========================================
- Coverage 94.01% 93.96% -0.05%
==========================================
Files 179 179
Lines 13144 13138 -6
==========================================
- Hits 12357 12345 -12
- Misses 787 793 +6
Continue to review full report at Codecov.
|
@dplewis the tests are not passing. I'm not sure if it is because of this PR or the others we've already commited. |
I think going forward we should ignore flaky test and test with network timeouts (transactions PG only and Parse.Push fixed here hopefully) until they are fixed. Otherwise reviews will take longer and we will be endlessly re running the CI in some cases. @mtrezza What do you think? |
Yes, that's an option. I was actually hoping that the annoyance of re-running tests incentivizes to fix the tests. My only concern would be that "hiding" the flakiness would lead to tests not being looked into and we end up piling them up, or even worse, hiding bugs. Didn't we fix most of the tests already? |
Yes we have. I don't know how many are left or which ones have been fixed. I assume there are about < 6 flaky tests left.
Annoyance is why I fixed the tests lol. |
Hah! There you go, why would we want to change a working strategy 😉 |
We will still keep track of the flaky tests since they will be easier to find now. The transaction network timeouts shouldn't be a show stopper @mtrezza What about this PR? |
If that a FIFO queue? I think that's a good approach, doesn't seem related to the transactions test fails? I restarted. |
Thats right, also seems like the CI is stuck. |
Ah good, I would still wait to merge until actions work again, just to be sure. |
🎉 This change has been released in version 5.0.0-beta.1 |
🎉 This change has been released in version 5.0.0 |
New Pull Request Checklist
Issue Description
The current promise chain can lead to unwanted behavior like older update overriding the latest and timeouts. This could also be the cause for a lot of flaky tests.
Approach
This reused the KeyPromiseQueue used in the RedisCacheAdapter. The operations for push and jobs are queue by the objectId of the status objects. Not only will it reduce bottlenecks for request but it is designed that way to allow maximum data consistency and prevent operations from overlapping.
TODOs before merging