Github user tdas commented on the pull request: https://github.com/apache/spark/pull/791#issuecomment-43795376 @cloud-fan @mridulm Aaah, I get it now. I knew I was missing something! I agree with @mridulm that this is a tricky lock structure and needs to be cleaner. Putting the `putLock.synchronized` inside the `ensureFreeSpace` is definitely better, as it co-locates the important locks together in the code. Less likely to be missed as I did. Maybe rename to `ensureFreeSpaceLock`? Or how about synchronizing on `this` ( that is, `def ensureFreeSpace(...): ReturnType = synchronized { ... } ` ? Also, please add a few more lines in the scaladoc of the `ensureFreeSpace` explaining this lock structure and the high-level "selection" and "marking" steps. You could give the higher level flow (select, mark, drop, exception handling) in the scala doc of `MemoryStore`, On a related note, have you run any long, rigorous test on this to make sure that (1) this new lock structure is not accidentally causing deadlocks (has happened before and was found only by running a long test)? (2) the memory limit is maintained all the time (to catch any race condition like i suggested even if remotely possible)?
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---