[GitHub] spark issue #18395: [SPARK-20655][core] In-memory KVStore implementation.

[jsoltren <gi...@git.apache.org>]

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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