srowen commented on a change in pull request #32534:
URL: https://github.com/apache/spark/pull/32534#discussion_r631855275
##########
File path: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala
##########
@@ -387,6 +388,14 @@ private[spark] class MemoryStore(
def remove(blockId: BlockId): Boolean = memoryManager.synchronized {
val entry = entries.synchronized {
+ entries.get(blockId) match {
+ case e: DeserializedMemoryEntry[_] => e.value.foreach(v => v match {
Review comment:
I think `.foreach(v => v match {` is redundant. Just `.foreach {`
##########
File path: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala
##########
@@ -387,6 +388,14 @@ private[spark] class MemoryStore(
def remove(blockId: BlockId): Boolean = memoryManager.synchronized {
val entry = entries.synchronized {
+ entries.get(blockId) match {
Review comment:
Doesn't remove already return what was removed? if so you can avoid .get
- just .remove here and match on it
##########
File path: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala
##########
@@ -387,6 +388,14 @@ private[spark] class MemoryStore(
def remove(blockId: BlockId): Boolean = memoryManager.synchronized {
val entry = entries.synchronized {
+ entries.get(blockId) match {
+ case e: DeserializedMemoryEntry[_] => e.value.foreach(v => v match {
+ case o: AutoCloseable =>
Review comment:
OK to this would cover plain Closeable too. I get the idea. On the one
hand I don't think we _know_ it's safe to close, but also can't think of a case
where you wouldn't, if it is closeable. Nothing else likely has a reference to
it anyway.
The downside here is that you spend time closing, and we're in a
synchronized block here, which is a little worrying. But it also may encounter
an exception. Do you want to try-catch for NonFatal here so that it doesn't
fail if close fails?
##########
File path: core/src/main/scala/org/apache/spark/storage/memory/MemoryStore.scala
##########
@@ -387,6 +388,14 @@ private[spark] class MemoryStore(
def remove(blockId: BlockId): Boolean = memoryManager.synchronized {
val entry = entries.synchronized {
+ entries.get(blockId) match {
+ case e: DeserializedMemoryEntry[_] => e.value.foreach(v => v match {
+ case o: AutoCloseable =>
+ o.close
+ case other =>
Review comment:
Nit: indentation, and just write _, not other
Also put parens on .close; it has side effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
