fix(core): fix request/response missmatch while prefetching #2843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2843 +/- ##
==========================================
+ Coverage 86.51% 87.89% +1.38%
==========================================
Files 138 138
Lines 9365 9402 +37
==========================================
+ Hits 8102 8264 +162
+ Misses 1263 1138 -125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
de2e96a to
37da579
Compare
37da579 to
066880a
Compare
There was a problem hiding this comment.
I would love to see the original test from @numb3r3 included in our CI/CD pipeline.
|
I merged #2837 into this branch. I removed the manual app for testing (https://github.com/jina-ai/jina/pull/2837/files#diff-ec88489890bdade23d4a2d2704d1e6b317e0fdd93298de42a695448fa26f6f86) as I think it doesnt belong in the test folder. The PyTest is still there: https://github.com/jina-ai/jina/pull/2843/files#diff-a1efda043579da8eecc8b8aa338e7358decb195883ef1c1804167f606c7c79a2 |
|
Storm test works now: https://github.com/jina-ai/jina/pull/2843/checks?check_run_id=2997162540 |
@jacobowitz wired, some errors are not captured by or maybe, this error log is normal one we expected. I think you should double check them. |
tests/integration/concurrent_clients/test_concurrent_clients.py
Outdated
Show resolved
Hide resolved
@numb3r3 |
It is indeed an upstream error seen in many tests. Would not pay attention in this context |
addressed all comments
This fixes the issue surfaced here: #2837
I added a message buffer during prefetching, which is used for matching the incoming responses from the flow to the correct requests from the clients. The clients are still waiting for their messages to be received from the Flow, which is done by waiting for the futures in the message buffer.
As prefetching now is stateful, I removed the mixin design of the prefetcher.
I also added a very simple unit test. The test sends an unexpected response (simulating other clients running concurrently) before sending the expected response. In current master this test fails, as we would incorrectly return the unexpected message. With the new functionality this message is dropped and the correct message is returned later.