While it is not a big issue, it is preferable to minimize surface for bugs
.. less code and state is preferable.
Keeping them around has no further value while having them wont allow
abstracting creation of the deserializeStream as a method as you observed.

Makes sense to remove them and have a single method to create the next
deserializeStream.
On Feb 6, 2014 2:48 AM, "andrewor14" <notifications@github.com> wrote:

> In
> core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala:
>
> > -            objectsRead = 0
> > -          }
> > -          objectsRead += 1
> > -          return deserializeStream.readObject().asInstanceOf[(K, C)]
> > -        } catch {
> > -          case e: EOFException =>
> > -            eof = true
> > -            cleanup()
> > +      try {
> > +        val item = deserializeStream.readObject().asInstanceOf[(K, C)]
> > +        objectsRead += 1
> > +        if (objectsRead == serializerBatchSize) {
> > +          batchStream = nextBatchStream()
> > +          compressedStream = blockManager.wrapForCompression(blockId, batchStream)
> > +          deserializeStream = ser.deserializeStream(compressedStream)
> > +          objectsRead = 0
> >          }
>
> It's true that we can get rid of batchStream and compressedStream, but
> keeping them around makes the composition of all the streams more explicit.
> Also, there aren't that many streams above batchStream, so the amount of
> duplicate code is very minimal and is unlikely to expand in the future. For
> these reasons I prefer to keep it as is (though this is really a borderline
> case).
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/apache/incubator-spark/pull/533/files#r9478696>
> .
>