Oak 11113 - MissingLastRevSeekerTest fix flaky test #1745
Conversation
| @@ -238,6 +238,11 @@ private void getBrokenSurrogateAndInitializeDifferentStores(DocumentStoreFixture | |||
| if (nodeStore != null) { | |||
| nodeStore.dispose(); | |||
| } | |||
| try { | |||
There was a problem hiding this comment.
In theory, disposing the node store should take care of this. We should find out why it doesn't.
There was a problem hiding this comment.
It is just a precocious adding. No truly impact.
There was a problem hiding this comment.
Ok, then please remove that change, because a) not needed, b) not related, and c) potentially confusing.
| @@ -249,7 +251,7 @@ public void dispose() { | |||
| } | |||
| dns.runBackgroundOperations(); | |||
| //seeker should return only non split documents | |||
| int docs = Iterables.size(seeker.getCandidates(0)); | |||
| int docs = Iterables.size(seeker.getCandidates(Objects.requireNonNull(dns.getRoot().getLastRevision().getRevision(dns.getClusterId())).getTimestamp())); | |||
There was a problem hiding this comment.
Is this related to the flakyness?
There was a problem hiding this comment.
Yes. I removed 0 and replaced it with proper revision version timestamp.
There was a problem hiding this comment.
Ok.
a) Could you put a short description of what was wrong into the ticket?
b) I would recommend that @egli, @rishabhdaim or @Joscorbe confirm that his is the correct change.
There was a problem hiding this comment.
0 was used to fetch all the documents. Using the timestamp of lastRevision would result in skipping some documents.
If there is no proper reason, I will revert the change.
There was a problem hiding this comment.
I was able to reproduce this test failure on my local. The root cause the is presence of document with id 1:/test in the RDB-H2.
Once I did mvn clean install this problem went away.
IMO, it is because of some earlier test(s) didn't cleanup properly rather than test.
There was a problem hiding this comment.
I think I found the Root cause.
These tests are failing due to the CompressedDocumentPropertyStateTest test not cleaning up properly.
On my local, it is causing either Mongo or RDB-H2 to fail.
CompressedDocumentPropertyStateTest creates the problematic node 1:/test causing test failures.
CompressedDocumentPropertyStateTest has been added recently which explains the recent failures of MissingLastRevSeekerTest.
@ionutzpi could you please look into this?
cc @Joscorbe @reschke
There was a problem hiding this comment.
Yes. I added CompressedDocumentPropertyStateTest changes to solve this issue. Julian told that documentfixturestore should close when closing nodestore(seems that this is not true).
There was a problem hiding this comment.
Calling fixture.dispose(); after disposing of nodeStore [1] would fix this problem for Mongo.
There was a problem hiding this comment.
It is not removing for RDB, I dont know the reason yet.
There was a problem hiding this comment.
@rishabhdaim if I add removeMe.add("1:/test"); after assert it will fix the issue. Do you agree with this change?
|
Created PR #1749 with proposed changes. |
|
@rishabhdaim - thanks for the analysis. This also explains why the test hasn't been flaky for a long time. Note that since a few weeks, AbstractDocumentStoreTest will check the NODES table before/after. So if a test fails like that, a check of the unit test log might be interesting. @ionutzpi - trust me; the RDB based tests assume that the test nodes are cleaned up (unless the test has been designed to purge tables). This is different from MongoDB because back then, I didn't want tests to nuke tables they did not create. This may have been the wrong approach, and we can discuss to change this. I'm just not sure it's worth the effort. |
|
Once it's clear which test is flaky, please adjust the issue title and make sure that the PR commit message matches. |
MissingLastRevSeekerTest fix flaky test getNonSplitDocs