Attempt preventing "record store state is being used for queries" during index rebuild#4011
Conversation
| * a future that holds a record store state read for a controlled duration, simulating | ||
| * a slow getSnapshotRecordCount FDB read. | ||
| */ | ||
| private static class SlowCountRecordStore extends FDBRecordStore { |
There was a problem hiding this comment.
Do you know how necessary is this class? Without your fix, do we hit the error somewhat reliably if we use RepeatedTest for a reasonably small number?
There was a problem hiding this comment.
I had hit this error every single time I've tried it without my fix.
I hadn't tried "repeat until success", though.
There was a problem hiding this comment.
I was more curious about how reliable the test becomes if we take it out as it would be nice not to introduce sleeps into the code, even for tests
There was a problem hiding this comment.
Oh, would you like me to eliminate the sleep?
(I do not know if before the change the tests could have consistently catch the problem without any sleep)
There was a problem hiding this comment.
Yeah, I was at least curious to know if the test could work without the sleeps. It's probably fine to leave it as is, though
…ring index rebuild Addresses two issues: 1. recordStoreStateRef.updateAndGet lambdas were non repeatble 2. rebuildIndexes preset futures items could havce performed index state read
+ Add rebuildIndexesWithMixedRecordCount
+ Add rebuildIndexesWithDisabledAndMixedStates
jjezra
force-pushed
the
rebuild_indexes_avoid_conflicts_small
branch
from
148a04f to
8faefd2
Compare
| * a future that holds a record store state read for a controlled duration, simulating | ||
| * a slow getSnapshotRecordCount FDB read. | ||
| */ | ||
| private static class SlowCountRecordStore extends FDBRecordStore { |
There was a problem hiding this comment.
I was more curious about how reliable the test becomes if we take it out as it would be nice not to introduce sleeps into the code, even for tests
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
I think this is better, and it should fix the bug I think that was manifesting between index builds and getting the index states. I've left some additional comments on the new fix, and I think there are a few outstanding from the last review
| * a future that holds a record store state read for a controlled duration, simulating | ||
| * a slow getSnapshotRecordCount FDB read. | ||
| */ | ||
| private static class SlowCountRecordStore extends FDBRecordStore { |
There was a problem hiding this comment.
Yeah, I was at least curious to know if the test could work without the sleeps. It's probably fine to leave it as is, though
3 participants
Addresses two issues: