Fix a bug in loadTasks which wasn't abiding by compare-and-swap semantics when updating TaskEntities #1134
Conversation
…tics when updating TaskEntities. Additionally, fix the PolarisTreeMapMetaStoreSessionImpl to avoid "cheating" by using its entire PolarisBaseEntit it happens to store in entitiesActive when performing a listing; in general, entitiesActive is assumed to *only* contain the name lookup records, so it is not explicitly updated unless a parent or name changes. The existing tests happened to pass because of a coincidence of several bugs: 1. Since the entitiesActive and entities happens to share the same PolarisBaseEntity instance by-reference, any in-place edits to "entities" *happens* to reflect in entitiesActive, but in general entities *should not be mutable* and usually a copy of an entity is mutated instead of mutating in place anyways 2. Because of the blind-writes in loadTasks, the "prepare entity" step where an instance is updated with a new entityVersion coincidentally immediately causes the main entities *and* entitiesAcitve slices to have the updated entity even before the call to writeEntity happens 3. Since TreeMap uses "synchronized" for runInTransaction, the loadTasksInParallel doesn't detect interleaving of threads So without the fix in the treemap store, simply fixing PolarisMetaStoreManagerImpl causes the test to run forever because tasks never get updated in entitiesActive so the same initial 5 tasks keep getting listed forever. Also fix the test to actually timeout eventually; the Future::get calls were willing to block forever before getting to the await. Reduce the wait time from 10 minutes to something more reasonable.
dennishuo
requested review from
adutra,
ashvina,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain and
ebyhr
as code owners
| ms.writeEntity(callCtx, task, false, originalTask); | ||
| updatedTask.setProperties( | ||
| PolarisObjectMapperUtil.serializeProperties(callCtx, properties)); | ||
| EntityResult result = updateEntityPropertiesIfNotChanged(callCtx, ms, null, updatedTask); |
There was a problem hiding this comment.
Question: why do we need to update task entities in a "load" call?
There was a problem hiding this comment.
Yeah, loadTasks probably is just a bad name. It is more correctly leaseTasks. Should I rename in this PR?
| @@ -1966,9 +1966,12 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( | |||
| }, | |||
| Function.identity()); | |||
|
|
|||
| List<PolarisBaseEntity> loadedTasks = new ArrayList<>(); | |||
There was a problem hiding this comment.
This loadTasks method appears to be called only from test code... Is that expected?
There was a problem hiding this comment.
I think @collado-mike had some plans for this. I believe it's what's needed next to have better than "best-effort in-memory attempt at cleanup of tables after purge".
For now I'm just trying to have the tests passing while implementing the NonTransactionalMetaStoreManager that also needs to support loadTasks to pass the base MetaStoreManager test :)
There was a problem hiding this comment.
On the plus side, task-leasing happens to be the most intensive concurrency set of unittests we have, so this TaskEntity stuff was what helped discover the missing pieces and bugs in my NonTransactionalMetaStoreManager implementation
Fix a bug in loadTasks which wasn't abiding by compare-and-swap semantics when updating TaskEntities
Additionally, fix the PolarisTreeMapMetaStoreSessionImpl to avoid "cheating" by using its entire PolarisBaseEntit it happens to store in entitiesActive when performing a listing; in general, entitiesActive is assumed to only contain the name lookup records, so it is not explicitly updated unless a parent or name changes.
The existing tests happened to pass because of a coincidence of several bugs:
So without the fix in the treemap store, simply fixing PolarisMetaStoreManagerImpl causes the test to run forever because tasks never get updated in entitiesActive so the same initial 5 tasks keep getting listed forever.
Also fix the test to actually timeout eventually; the Future::get calls were willing to block forever before getting to the await. Reduce the wait time from 10 minutes to something more reasonable.