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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
