[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread shivaram 
Github user shivaram commented on a diff in the pull request:

https://github.com/apache/spark/pull/15433#discussion_r83043023
  
--- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala 
---
@@ -263,18 +264,19 @@ private[r] object JVMObjectTracker {
 
   // TODO: This map should be thread-safe if we want to support multiple
   // connections at the same time
-  private[this] val objMap = new HashMap[String, Object]
+  private[this] val objMap: ConcurrentMap[String, Object] =
+new MapMaker().weakValues().makeMap[String, Object]()
 
   // TODO: We support only one connection now, so an integer is fine.
   // Investigate using use atomic integer in the future.
   private[this] var objCounter: Int = 0
 
   def getObject(id: String): Object = {
-objMap(id)
+objMap.get(id)
   }
 
   def get(id: String): Option[Object] = {
-objMap.get(id)
+Option(objMap.get(id))
   }
 
   def put(obj: Object): String = {
--- End diff --

There is a another JIRA related to thread safety of this class 
https://issues.apache.org/jira/browse/SPARK-17823 - It might be good address 
that in this PR ?


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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/15433#discussion_r82961129
  
--- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala 
---
@@ -263,18 +264,19 @@ private[r] object JVMObjectTracker {
 
   // TODO: This map should be thread-safe if we want to support multiple
   // connections at the same time
-  private[this] val objMap = new HashMap[String, Object]
+  private[this] val objMap: ConcurrentMap[String, Object] =
+new MapMaker().weakValues().makeMap[String, Object]()
 
   // TODO: We support only one connection now, so an integer is fine.
   // Investigate using use atomic integer in the future.
   private[this] var objCounter: Int = 0
 
   def getObject(id: String): Object = {
-objMap(id)
+objMap.get(id)
   }
 
   def get(id: String): Option[Object] = {
-objMap.get(id)
+Option(objMap.get(id))
   }
 
   def put(obj: Object): String = {
--- End diff --

It kind of seems like this class is meant to be thread-safe, but this 
isn't. You could use `AtomicLong` and `getAndIncrement`?


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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/15433#discussion_r82960952
  
--- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala 
---
@@ -263,18 +264,19 @@ private[r] object JVMObjectTracker {
 
   // TODO: This map should be thread-safe if we want to support multiple
   // connections at the same time
-  private[this] val objMap = new HashMap[String, Object]
+  private[this] val objMap: ConcurrentMap[String, Object] =
--- End diff --

Not that it matters a lot, but do you need the reference type?


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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread srowen 
Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/15433#discussion_r82960799
  
--- Diff: core/src/main/scala/org/apache/spark/api/r/RBackendHandler.scala 
---
@@ -284,7 +286,7 @@ private[r] object JVMObjectTracker {
 objId
   }
 
-  def remove(id: String): Option[Object] = {
+  def remove(id: String) {
--- End diff --

Although it's normal to return the object that was removed from map 
methods, if this isn't desired, I'd declare this as `: Unit` to be clear


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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread techaddict 
GitHub user techaddict reopened a pull request:

https://github.com/apache/spark/pull/15433

[SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may 
leak JVM objects

## What changes were proposed in this pull request?
Use weak reference in JVMObjectTracker.objMap because it may leak JVM 
objects

## How was this patch tested?
existing tests

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/techaddict/spark SPARK-17822

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/15433.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #15433


commit 7023c40a99eaa81ee7bcd202a4b74df811d0cfc7
Author: Sandeep Singh 
Date:   2016-10-10T11:54:34Z

[SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may 
leak JVM objects

commit 69845947df62187eb40f3cc6468b52e38bdab897
Author: Sandeep Singh 
Date:   2016-10-10T13:23:56Z

Merge branch 'master' into SPARK-17822

commit 995611d75351d24907ce2b22e7d33752cc803da3
Author: Sandeep Singh 
Date:   2016-10-11T13:13:09Z

Merge branch 'master' into SPARK-17822

commit 8e763bef78fe147e84e1771f237a75ff42780705
Author: Sandeep Singh 
Date:   2016-10-12T06:33:26Z

fix for failures

commit 7d50d84f90fcda9e5dec79c9be834870c83443c4
Author: Sandeep Singh 
Date:   2016-10-12T06:34:23Z

Merge branch 'master' into SPARK-17822




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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-12 Thread techaddict 
Github user techaddict closed the pull request at:

https://github.com/apache/spark/pull/15433


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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #15433: [SPARK-17822] Use weak reference in JVMObjectTrac...

2016-10-11 Thread techaddict 
GitHub user techaddict opened a pull request:

https://github.com/apache/spark/pull/15433

[SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may 
leak JVM objects

## What changes were proposed in this pull request?
Use weak reference in JVMObjectTracker.objMap because it may leak JVM 
objects

## How was this patch tested?
existing tests

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/techaddict/spark SPARK-17822

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/15433.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #15433


commit 7023c40a99eaa81ee7bcd202a4b74df811d0cfc7
Author: Sandeep Singh 
Date:   2016-10-10T11:54:34Z

[SPARK-17822] Use weak reference in JVMObjectTracker.objMap because it may 
leak JVM objects

commit 69845947df62187eb40f3cc6468b52e38bdab897
Author: Sandeep Singh 
Date:   2016-10-10T13:23:56Z

Merge branch 'master' into SPARK-17822




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

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org