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.
---
