[SPARK-17839][CORE] Use Nio's directbuffer instead of BufferedInputStream in order to avoid additional copy from os buffer cache to user buffer #15408

sitalkedia · 2016-10-09T18:06:51Z

What changes were proposed in this pull request?

Currently we use BufferedInputStream to read the shuffle file which copies the file content from os buffer cache to the user buffer. This adds additional latency in reading the spill files. We made a change to use java nio's direct buffer to read the spill files and for certain pipelines spilling significant amount of data, we see up to 7% speedup for the entire pipeline.

How was this patch tested?

Tested by running the job in the cluster and observed up to 7% speedup.

…ffer to read the spill files in order to avoid additional copy

sitalkedia · 2016-10-09T18:08:44Z

cc - @rxin

srowen

Seems conceptually sound and the benchmark shows good results. I wonder, is this not something already available in the JDK? I could only find this from the JDK, which doesn't do internal buffering, but does some more stuff that might be worth comparing notes with: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/sun/nio/ch/ChannelInputStream.java/

srowen · 2016-10-09T18:38:33Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+ */
+public final class NioBasedBufferedFileInputStream extends InputStream {
+
+  ByteBuffer bb;


private final

srowen · 2016-10-09T18:39:00Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+
+  FileChannel ch;
+
+  public NioBasedBufferedFileInputStream(File file, int bufferSize) throws IOException {


Would it be reasonable to establish a default constructor with some default buffer size, like BufferedInputStream?

that makes sense, changed accordingly.

srowen · 2016-10-09T19:16:59Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+
+  public NioBasedBufferedFileInputStream(File file, int bufferSize) throws IOException {
+    bb = ByteBuffer.allocateDirect(bufferSize);
+    FileInputStream f = new FileInputStream(file);


I'm not entirely sure about this, but seems so from reading the source: closing the channel won't free the file descriptor? can you just hang on to this ref as well to close it in close() for completeness?

Better yet, can't we just use FileChannel.open(...) and not bother with the stream?

changed accordingly.

srowen · 2016-10-09T19:17:27Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+    bb.flip();
+  }
+
+  public boolean refill() throws IOException {


srowen · 2016-10-09T19:23:25Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+  public void close() throws IOException {
+    ch.close();
+  }
+}


I think you'll want to implement skip() and available() too.

skip() is already implemented by underlying InputStream so that is not needed. Implemented available() .

skip() in InputStream will call read(), this is not the optimal solution

Although, we are not using skip() anywhere, but I implemented an optimal version of it anyways.

srowen · 2016-10-09T19:24:34Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeSorterSpillReader.java


-    final BufferedInputStream bs =
-        new BufferedInputStream(new FileInputStream(file), (int) bufferSizeBytes);
+    final NioBasedBufferedFileInputStream bs =


BTW I think IndexShuffleBlockResolver, RowQueue have instances that could use a similar treatment.

Can this reference be InputStream?

Yes, there are other instance in the code where we are using BufferedInputStream. I will file raise another PR to replace those with NioBasedBufferedFileInputStream.

SparkQA · 2016-10-09T19:47:23Z

Test build #66610 has finished for PR 15408 at commit bd741fa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public final class NioBasedBufferedFileInputStream extends InputStream

sitalkedia · 2016-10-09T20:53:04Z

Seems conceptually sound and the benchmark shows good results. I wonder, is this not something already available in the JDK?

Yes, I looked around a bit and unfortunately, I could not find any implementation in JDK which support buffering based nio read.

SparkQA · 2016-10-09T22:59:05Z

Test build #66612 has finished for PR 15408 at commit 12c9ded.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-10-10T00:20:58Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+ * but does not support buffering.
+ *
+ */
+public final class NioBasedBufferedFileInputStream extends InputStream {


You should add a test suite specifically for this.

Added a test suite for this.

Great - thanks. Just FYI in general I find ScalaTest to be a better framework than JUnit, so it is a good idea to write tests in Scala even if the class is implemented in Java. Not a big deal though.

rxin · 2016-10-10T00:21:33Z

Can you also expand on what the 7% means? Is it some workload end-to-end that's been improved by 7%, or the sorting itself improves by 7%?

witgo · 2016-10-10T01:43:39Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+    bb = ByteBuffer.allocateDirect(bufferSize);
+    ch = FileChannel.open(file.toPath(), StandardOpenOption.READ);
+    ch.read(bb);
+    bb.flip();


ch.read(bb); can be removed

removed, thanks!

sitalkedia · 2016-10-10T03:45:44Z

Can you also expand on what the 7% means? Is it some workload end-to-end that's been improved by 7%, or the sorting itself improves by 7%?

The perf improvement was end-to-end which means the sorting improvement is definitely more than that. Also, changed the PR description to make that clear.

SparkQA · 2016-10-10T06:09:57Z

Test build #66621 has finished for PR 15408 at commit 856593a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-10-10T07:34:10Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+ */
+public final class NioBasedBufferedFileInputStream extends InputStream {
+
+  private static int DEFAULT_BUFFER_SIZE = 8192;


we don't do this consistently, but would be useful to rename this DEFAULT_BUFFER_SIZE_BYTES so it is clear what the unit is

rxin · 2016-10-10T07:34:18Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+
+  private final FileChannel fileChannel;
+
+  public NioBasedBufferedFileInputStream(File file, int bufferSize) throws IOException {


here too, bufferSizeInBytes

rxin · 2016-10-10T07:36:54Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+    this(file, DEFAULT_BUFFER_SIZE);
+  }
+
+  private boolean refill() throws IOException {


document this function and explain what the ret value means?

rxin · 2016-10-10T07:37:58Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+  }
+
+  @Override
+  public int read(byte[] b, int off, int len) throws IOException {


off -> offset

rxin · 2016-10-10T07:38:22Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+  public int available() throws IOException {
+    return byteBuffer.remaining();
+  }
+


remove one blank line

rxin · 2016-10-10T07:39:34Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+  }
+
+  @Override
+  public int available() throws IOException {


one question: how is this used? AFAIK InputStream always return 0 for this. Is this used somewhere else for performance improvements?

It's a standard API method for InputStream, so if something does call it, it's better to return a useful answer because it's pretty easy to do so.

I agree, InputStream simply has a stub impl (just like read(byte[]) loops through read). Providing .remaining() can be useful

rxin · 2016-10-10T07:40:00Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+
+  @Override
+  public long skip(long n) throws IOException {
+    if(n < 0L) {


nit: add a space after if

rxin · 2016-10-10T07:41:58Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+      fileChannel.position(size);
+      return size - currentFilePosition;
+    }
+    else {


move else to previous line

rxin · 2016-10-10T07:42:35Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+    return skipFromFileChannel(toSkip);
+  }
+
+  private long skipFromFileChannel(long n) throws IOException {


maybe just inline this in the previous method, and add an else case so both cases are indented the same level?

actually never mind this comment

rxin · 2016-10-10T07:43:13Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+
+  @Override
+  public void close() throws IOException {
+    fileChannel.close();


do we need to free the buffer or anything?

No, there is a cleaner in DirectByteBuffer which frees up the memory when the object is garbage collected.

Ah OK. I vaguely remember Spark ran into an issue in ~ Spark 1.0 that the direct bytebuffers were not cleared fast enough (because the life cycle is decoupled from GC). We introduced StorageUtils.dispose and made buffer clearing more aggressive. It might be good to call that here too, just in case.

I see...that makes sense. Added an explicit call to cleanup the byte buffer.

rxin · 2016-10-10T07:44:06Z

core/src/test/java/org/apache/spark/io/NioBasedBufferedFileInputStreamSuite.java

+  }
+
+  @Test
+  public void testBytesSkipped() throws IOException {


can we add a test case for negative skips?

rxin · 2016-10-10T07:44:37Z

core/src/test/java/org/apache/spark/io/NioBasedBufferedFileInputStreamSuite.java

+  }
+
+  @Test
+  public void testReadMultipleBytes() throws IOException {


we should construct a test case that triggers the code path in skipFromFileChannel very explicitly

Good idea. Actually I discovered a bug with my previous implementation after adding this test case, which I have fixed.

This reminds me --- we should add test coverage reporting to Spark so it becomes obvious what branches are not tested sufficiently.

rxin · 2016-10-10T07:44:59Z

Looks pretty good overall. I left some comments mostly on styling, documentation, and test cases.

srowen

Looking good; I don't know that it makes sense to change other instances separately? Changing all instances of buffering a file input stream seems like one coherent logical change (but the titles need to be updates)

srowen · 2016-10-10T07:48:01Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+  }
+
+  @Override
+  public int available() throws IOException {


It's a standard API method for InputStream, so if something does call it, it's better to return a useful answer because it's pretty easy to do so.

srowen · 2016-10-10T07:48:21Z

core/src/main/java/org/apache/spark/io/NioBasedBufferedFileInputStream.java

+    if(n < 0L) {
+      return 0L;
+    }
+    if (byteBuffer.remaining() > n) {


= n ? or did I miss something obvious?

If we have enough data in the bytebuffer to skip then we can skip from the buffer and no need to skip from the file. Looks good to me.

Oh, my comment rendered incorrectly. I'm asking if the condition can be >= n. If there are exactly n bytes remaining then you can skip the entire buffer, right? it need not be > n?

Yes, we can change the condition to be >=n. Although the behavior remains the same because skipping 0 bytes from the underlying file channel is essentially a no-op. But changed it avoid confusion.

srowen · 2016-10-10T07:49:40Z

core/src/test/java/org/apache/spark/io/NioBasedBufferedFileInputStreamSuite.java

+ */
+public class NioBasedBufferedFileInputStreamSuite {
+
+  byte[] randomBytes;


Nit: private should always be the default

sitalkedia · 2016-10-10T17:28:00Z

Thanks @srowen, @rxin for the review comments. I have addressed them.

SparkQA · 2016-10-10T17:29:57Z

Test build #66667 has finished for PR 15408 at commit 3d48a16.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-11T19:33:56Z

Test build #66749 has finished for PR 15408 at commit 30173fa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sitalkedia · 2016-10-11T22:14:25Z

jenkins retest this please.

zsxwing

Please add synchronized to the public methods except close (the current close method is already thread-safe). The javadoc of InputStream says nothing about thread-safety but the implementation like ChannelInputStream, ByteArrayInputStream, BufferedInputStream implies that.
A minor behavior change is NioBufferedFileInputStream doesn't close fd in finalize like FileInputStream. But that's not a big deal since the user should do it instead of counting on GC.

zsxwing · 2016-10-11T21:50:53Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+  private long skipFromFileChannel(long n) throws IOException {
+    long currentFilePosition = fileChannel.position();
+    long size = fileChannel.size();
+    if (currentFilePosition + n > size) {


nit: use n > size - currentFilePosition to avoid overflow (e.g., n is Long.MAX_VALUE).

good point, changed accordingly.

zsxwing · 2016-10-11T22:35:18Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+
+  @Override
+  public int read(byte[] b, int offset, int len) throws IOException {
+    if (!refill()) {


nit: please add the defense codes like BufferedInputStream

if ((off | len | (off + len) | (b.length - (off + len))) < 0) { throw new IndexOutOfBoundsException(); } else if (len == 0) { return 0; }

SparkQA · 2016-10-12T00:08:17Z

Test build #66765 has finished for PR 15408 at commit 30173fa.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

I also missed why close() shouldn't be synchronized. You wouldn't want it to take place during a read, right?

I think a finalizer wouldn't hurt here because FileInputStream does it. It's exactly because people may not properly close the stream.

WDYT @zsxwing ?

Getting quite close here I think.

srowen · 2016-10-12T16:06:37Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

    if (!refill()) {
      return -1;
    }
+    if ((offset | len | (offset + len) | (b.length - (offset + len))) < 0) {


I think this must be a typo... this is a bitwise-or of a bunch of ints. I think maybe the example was given as a sort of pseudo code. offset and len can't be negative and their sum shouldn't exceed the array lenght, but that seems like it. 'else' below isn't really needed, but that's a nit. Actually, you want to check whether len is after updating on line 94 right? if no data is available you also return 0 without a no-op read?

@srowen I think this check is correct. It checks if any of these values is negative.

offset and len can't be negative and their sum shouldn't exceed the array length.

It's possible. offset and len are set by the caller.

@sitalkedia Could you move the argument check before refill? If the argument is invalid, we should always throw an exception instead of returning -1.

Removed the 'else' part and moved the argument check before refill as suggested.

@zsxwing oh I get it, you're really just checking if any top bit is set. Hm, that seems fairly obscure compared to just checking the arguments. Sure it's a little more code, but it's readable and i don't think this is performance critical? I hadn't seen this before and had to think a while to get what it does.

Yes, I'm agreeing that offset and len can't be allowed to be negative, not that it can't be specified by the caller as something negative.

@srowen this line comes from BufferedInputStream. I'm ok with rewriting it for readability.

Oh I see. Well it's reasonable to keep it then, but I'd at least add a comment I think. I didn't recognize this at all until I thought about it for a good few minutes.

@srowen - Agreed, the condition was little confusing. Changed it to make it clearer.

SparkQA · 2016-10-12T18:27:48Z

Test build #66830 has finished for PR 15408 at commit 439cead.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-10-12T19:08:11Z

I also missed why close() shouldn't be synchronized. You wouldn't want it to take place during a read, right?

Good point. I was thinking FileChannel.close is safe but didn't consider this case.

I think a finalizer wouldn't hurt here because FileInputStream does it. It's exactly because people may not properly close the stream.

Agreed that it doesn't hurt the performance since FileInputStream does it.

SparkQA · 2016-10-13T04:14:02Z

Test build #66860 has finished for PR 15408 at commit b74fb36.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-14T20:08:47Z

Test build #66972 has finished for PR 15408 at commit 769b3f0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sitalkedia · 2016-10-14T20:30:51Z

jenkins retest this please.

SparkQA · 2016-10-14T22:58:01Z

Test build #66978 has finished for PR 15408 at commit 769b3f0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

Now we're down to pretty trivial changes, thank you. This will be a nice win and I am glad we've scrutinized it because it will be in a lot of critical paths.

srowen · 2016-10-15T10:18:49Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+ * TODO: support {@link #mark(int)}/{@link #reset()}
+ *
+ */
+@ThreadSafe


I only see one location in the codebase where we use this annotation, and I think we probably shouldn't use it at all if not used consistently.

Alright, removed it for consistency.

srowen · 2016-10-15T10:18:57Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+@ThreadSafe
+public final class NioBufferedFileInputStream extends InputStream {
+
+  private static int DEFAULT_BUFFER_SIZE_BYTES = 8192;


Oops, forgot to say this should be final

srowen · 2016-10-15T10:19:25Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+ * {@link sun.nio.ch.ChannelInputStream} supports reading a file using nio,
+ * but does not support buffering.
+ *
+ * TODO: support {@link #mark(int)}/{@link #reset()}


I don't really care, but this could be a comment inside the class rather than user-facing. In fact I don't even know it's a to-do.

Okay, I removed the TODO here.

srowen · 2016-10-15T10:20:44Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+
+  @Override
+  public synchronized int read(byte[] b, int offset, int len) throws IOException {
+    if (offset < 0 || len < 0 || (offset + len) < 0 || (b.length - (offset + len)) < 0) {


Hardly matters, but now that this condition has been made more explicit, then final condition is simpler as offset + len > b.length

We still need to check if offset and len is less than 0 right? Removed theoffset + len < 0 condition because that is covered in the last condition (b.length - (offset + len)) < 0

Ignore my previous comment, we still need it.

srowen · 2016-10-15T10:21:08Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+
+  @Override
+  protected void finalize() throws IOException {
+      close();


Nit: 2-space indent not 4

good eye, wonder why the checkstyle did not catch it though?

srowen · 2016-10-15T14:11:27Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+
+import org.apache.spark.storage.StorageUtils;
+
+import javax.annotation.concurrent.ThreadSafe;


Nit^2 : no longer needed as an import

srowen · 2016-10-15T14:12:17Z

core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java

+
+  @Override
+  public synchronized int read(byte[] b, int offset, int len) throws IOException {
+    if (offset < 0 || len < 0 || (b.length - (offset + len)) < 0) {


Ah no I think that condition was needed. I mean: if (offset < 0 || len < 0 || offset + len < 0 || offset + len > b.length) {

srowen

LGTM pending tests

SparkQA · 2016-10-15T16:28:42Z

Test build #67014 has finished for PR 15408 at commit f1f108f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-15T16:37:33Z

Test build #67016 has finished for PR 15408 at commit 5306fb0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-10-17T10:55:45Z

Going once, going twice, any more comments?

zsxwing · 2016-10-17T18:02:43Z

LGTM. Merging to master. Thanks!

…ream in order to avoid additional copy from os buffer cache to user buffer ## What changes were proposed in this pull request? Currently we use BufferedInputStream to read the shuffle file which copies the file content from os buffer cache to the user buffer. This adds additional latency in reading the spill files. We made a change to use java nio's direct buffer to read the spill files and for certain pipelines spilling significant amount of data, we see up to 7% speedup for the entire pipeline. ## How was this patch tested? Tested by running the job in the cluster and observed up to 7% speedup. Author: Sital Kedia <skedia@fb.com> Closes apache#15408 from sitalkedia/skedia/nio_spill_read.

[SPARK-17839][CORE] UnsafeSorterSpillReader should use Nio's directbu…

bd741fa

…ffer to read the spill files in order to avoid additional copy

srowen requested changes Oct 9, 2016

View reviewed changes

Addressed review comments

12c9ded

rxin reviewed Oct 10, 2016

View reviewed changes

witgo reviewed Oct 10, 2016

View reviewed changes

Added test suite

856593a

rxin reviewed Oct 10, 2016

View reviewed changes

srowen requested changes Oct 10, 2016

View reviewed changes

Added more tests and addressed review comments

3d48a16

zsxwing requested changes Oct 11, 2016

View reviewed changes

Make the class thread safe and address other review comments

439cead

srowen requested changes Oct 12, 2016

View reviewed changes

Make close function call synchronized and address other review comments

b74fb36

Minor change to improve readability of bound checking condition

769b3f0

srowen requested changes Oct 15, 2016

View reviewed changes

Minor nits

f1f108f

srowen requested changes Oct 15, 2016

View reviewed changes

Sital Kedia added 2 commits October 15, 2016 07:16

Simplify the bound checking

4a9bc2f

Fix boundchecking again

5306fb0

srowen approved these changes Oct 15, 2016

View reviewed changes

asfgit closed this in c7ac027 Oct 17, 2016

ConeyLiu mentioned this pull request Jan 21, 2018

[SPARK-22838][Core] Avoid unnecessary copying of data #20026

Closed


		FileChannel ch;

		public NioBasedBufferedFileInputStream(File file, int bufferSize) throws IOException {


		private final FileChannel fileChannel;

		public NioBasedBufferedFileInputStream(File file, int bufferSize) throws IOException {


		import org.apache.spark.storage.StorageUtils;

		import javax.annotation.concurrent.ThreadSafe;

[SPARK-17839][CORE] Use Nio's directbuffer instead of BufferedInputStream in order to avoid additional copy from os buffer cache to user buffer #15408

[SPARK-17839][CORE] Use Nio's directbuffer instead of BufferedInputStream in order to avoid additional copy from os buffer cache to user buffer #15408

Uh oh!

Conversation

sitalkedia commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sitalkedia commented Oct 9, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 9, 2016

Uh oh!

sitalkedia commented Oct 9, 2016

Uh oh!

SparkQA commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Oct 10, 2016

Uh oh!

witgo Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sitalkedia commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sitalkedia commented Oct 9, 2016 •

edited

Loading

srowen Oct 9, 2016 •

edited

Loading

witgo Oct 10, 2016 •

edited

Loading

sitalkedia commented Oct 10, 2016 •

edited

Loading

mridulm Oct 10, 2016 •

edited

Loading