ISPN-14119 Filter null from evicted entities for distributed queries #10396
Conversation
There was a problem hiding this comment.
From the sound of the bug, my guess would be is the entry is in the index, the user runs the query which finds the reference id, and concurrently the entry is removed due to expiration and thus when it tries to read the referenced key it is no longer present.
I would suggest as a guaranteed way to reproduce this issue I would recommend using Mocks.blockingMock replacing the component that does the actual object lookup (if possible). This allows you to fire the query in one thread and wait until it gets to where it is looking up the entries and block the operation. Then you can increment the time service and expire the entry. Then let the query continue and it should face the issue every time you run it without having to do loops or sleeps.
| public void test() throws Exception { | ||
| cache.put(1, new Game("Ultima IV: Quest of the Avatar", "It is the first in the \"Age of Enlightenment\" trilogy ...")); | ||
|
|
||
| Thread.sleep(3000); |
There was a problem hiding this comment.
You can use a controlled time service instead, to not have this sleep.
https://github.com/infinispan/infinispan/blob/main/query/src/test/java/org/infinispan/query/continuous/ContinuousQueryTest.java#L30
But please use the non deprecated ControlledTimeService.
| } | ||
|
|
||
| @Test | ||
| public void test() throws Exception { |
There was a problem hiding this comment.
We should probably have a few tests in here, testing running the query before it expires and after.
| .set("name", "Game " + key) | ||
| .set("description", "This is the game " + i + "# of a series"); | ||
|
|
||
| Thread.sleep(40); |
There was a problem hiding this comment.
We should not have any sleep calls in our tests, please use the time service as mentioned prior.
| .set("name", "Game " + key) | ||
| .set("description", "This is the game " + i + "# of a series"); | ||
|
|
||
| Thread.sleep(40); |
There was a problem hiding this comment.
We should not have any sleep calls in our tests, please use the time service as mentioned prior.
|
Thanks for review @wburns!
exactly! I introduced the controlled time service and ran the query before and after the expiration. |
Not sure what you mean by all tests. But I would say if you are using a test that has expiration, you should definitely use a controlled time service, so you don't have the tests take too long and you control the behavior at a much granular level and should be very easily repeatable. Regarding blockingMock, that is only needed if there is a step in between. Does the test as you have it reproduce the issue every time it is invoked? Or does it fail rarely/intermittently? If so then we will want to inject the blockingMock to be sure to reproduce it easily enough. |
I don't think so. Thus I think we can avoid using the blockingMock IIUC. Thanks again @wburns |
Seems fine, I am just wondering why we don't have the query module test anymore. Seems a bit much with the rest test to test this specific bug. I am fine having both, but can we resurrect the old test you had as well? |
|
Thank @wburns. |
|
Integrated into main, thanks @fax4ever ! |
|
thank you Will! |
https://issues.redhat.com/browse/ISPN-14119