Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3629#issuecomment-75179521
@suyanNone @liyezhang556520 sorry for slipping on this again. I just took a
close look at the patch again and I am convinced that it does the right thing.
I will leave a few more comments inline and hopefully soon after that we can
merge this into master.
> But from my point of view, the value of previousMemoryReserved should be
always 0
I believe it is 0 only if we only call `unrollSafely` once per task. That
may be true now (I haven't looked too closely to verify this), but we don't
want to make that an assumption that will constrain the use of this method in
the future. This method is intended to be a fairly lightweight utility method
shared by multiple components of the Spark code (currently both the block
manager and the cache manager), and imposing the once-per-task constraint might
complicate things downstream.
> I will still releasePendingUnrollMemory() before ensureFreeSpace this line
Because of the accounting lock it actually doesn't matter too much which
line it is. Even if we release it before we actually cache the block, other
threads still won't be able to get our released memory. However, I still prefer
to release it on this
[line](https://github.com/apache/spark/blob/4e1f12d997426560226648d62ee17c90352613e7/core/src/main/scala/org/apache/spark/storage/MemoryStore.scala#L355)
because it is easier to reason about. Semantically it makes more sense for us
to release a resource after we have finished using it. In this case, we should
release the pending memory after we have finished trying to cache it.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
