[GitHub] spark issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user vanzin commented on the issue: https://github.com/apache/spark/pull/18395 I'll merge this and address Reynold's suggestions in a follow-up commit. Merging to master. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user vanzin commented on the issue: https://github.com/apache/spark/pull/18395 I plan to push this soon to unblock the next steps of the project. I'll leave this open here a little bit in case anyone is still thinking about commenting. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/18395 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80222/ Test PASSed. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/18395 Merged build finished. Test PASSed. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/18395 **[Test build #80222 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80222/testReport)** for PR 18395 at commit [`76cb291`](https://github.com/apache/spark/commit/76cb291d21ead292e5dda2d20a206492e23a3120). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/18395 **[Test build #80222 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80222/testReport)** for PR 18395 at commit [`76cb291`](https://github.com/apache/spark/commit/76cb291d21ead292e5dda2d20a206492e23a3120). --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user jsoltren commented on the issue: https://github.com/apache/spark/pull/18395 This looks fine to me. Though, I did have to ask Marcelo a number of questions along the way to clarify what all was going on here. Maybe some of the points I mention below should make their way back to the code in the form of comments. I might suggest a couple of minor code changes from the discussion below. The idea here is that, in the in memory store, arrays of Java primitive types can be used as keys. (Given an array, use the entire array as a key.) But, since primitive types are not Java objects, we need some code to make this happen. ArrayWrappers.java is the place where this happens. Each primitive type that is supported, needs its own implementation. Not all the primitive types are covered here. It is unfortunate that there is a bunch of duplicated code in ArrayWrappers.java. Marcelo tells me that Java does not support generic methods for primitive types, so, a bunch of code has to be copy-pasted. I don't see a good way around this. The testing in ArrapWrappersSuite.java seems adequate, though I'd like to see coverage for long[] here as well, and in the future, for any further primitive types that are supported in ArrayWrappers. The core of the InMemoryStore implementation is an implementation of KVStore that wraps operations on an InstanceList, which itself wraps operations on a ConcurrentMap. This seems like a reasonable choice for this application. I don't think there is much that we can do about this, but, this seems like a lot of code for a nice presentation of a ConcurrentMap. I think this is just Java, since we need an Iterator, a View, and an inner class to hold records. I liked the use of a Java 1.8 anonymous function in InMemoryStore.write(). All the tests pass when run locally. This change only defines the in-memory KVStore implementation. It does not wire it in to anything. Nits: IntelliJ had a bunch of warnings about replacing for with foreach, and about making public methods package-private. ArrayWrappers.java: IntelliJ claims the following import statement appears to be unused: import com.google.common.base.Objects; Line 26: I would do s/used as a key/used as keys/. InMemoryStore.java: IntelliJ claims the following import statements appear to be unused: import java.lang.reflect.Array; import java.util.Arrays; import java.util.Comparator; import java.util.TreeMap; --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user vanzin commented on the issue: https://github.com/apache/spark/pull/18395 (Also, because this PR is not adding this package, just adding classes to it, I'd rather do any package name change in a subsequent 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user vanzin commented on the issue: https://github.com/apache/spark/pull/18395 @rxin I wasn't planning on this being exposed at the moment; maybe in the future, since this is sort of a "natural" extension point to allow different storage backends for the UI / SHS. I'm also not sure how to make something not "publicly accessible"; I'm pretty sure I didn't add this to any doc target, unless it's automatically picked up (i.e. I did not touch the `Unidoc` object in the sbt build). Should I still go and add the `@Private` annotations to public classes? (That's not done by other private modules, such as the networking module, although that one predates `@Private`.) I'm not super excited about changing package names at this point, but if you feel strongly about it it's not a big deal. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user rxin commented on the issue: https://github.com/apache/spark/pull/18395 Is this going to be exposed? Either way, we should find something like spark.util.kvstore package rather than a top level package. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/18395 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/78479/ Test PASSed. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/18395 Merged build finished. Test PASSed. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/18395 **[Test build #78479 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78479/testReport)** for PR 18395 at commit [`cb96ac1`](https://github.com/apache/spark/commit/cb96ac16f17c2abc788c2e3ba7ccf3acb22ea785). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- 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 issue #18395: [SPARK-20655][core] In-memory KVStore implementation.
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/18395 **[Test build #78479 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/78479/testReport)** for PR 18395 at commit [`cb96ac1`](https://github.com/apache/spark/commit/cb96ac16f17c2abc788c2e3ba7ccf3acb22ea785). --- 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
