You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@orc.apache.org by se...@apache.org on 2018/03/09 01:19:53 UTC

[1/3] orc git commit: ORC-310 better error handling for codec

Repository: orc
Updated Branches:
  refs/heads/orc-310 [created] c6a58c670


ORC-310
better error handling for codec


Project: http://git-wip-us.apache.org/repos/asf/orc/repo
Commit: http://git-wip-us.apache.org/repos/asf/orc/commit/d9727cb3
Tree: http://git-wip-us.apache.org/repos/asf/orc/tree/d9727cb3
Diff: http://git-wip-us.apache.org/repos/asf/orc/diff/d9727cb3

Branch: refs/heads/orc-310
Commit: d9727cb36c9b1e9f1c045e29d928401a680062cb
Parents: 411e633
Author: sergey <se...@apache.org>
Authored: Wed Feb 28 16:00:51 2018 -0800
Committer: sergey <se...@apache.org>
Committed: Wed Feb 28 16:00:51 2018 -0800

----------------------------------------------------------------------
 .../java/org/apache/orc/impl/OrcCodecPool.java  | 15 +++++-
 .../src/java/org/apache/orc/impl/OrcTail.java   |  4 +-
 .../org/apache/orc/impl/PhysicalFsWriter.java   |  7 ++-
 .../java/org/apache/orc/impl/ReaderImpl.java    | 12 +++--
 .../org/apache/orc/impl/RecordReaderImpl.java   |  6 ++-
 .../org/apache/orc/impl/RecordReaderUtils.java  | 54 +++++++++++++++-----
 .../java/org/apache/orc/impl/WriterImpl.java    |  6 ++-
 7 files changed, 80 insertions(+), 24 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
index 56b9896..34f0bd4 100644
--- a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
+++ b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
@@ -18,10 +18,8 @@
 package org.apache.orc.impl;
 
 import java.util.concurrent.ConcurrentHashMap;
-
 import java.util.ArrayList;
 import java.util.List;
-
 import org.apache.orc.CompressionCodec;
 import org.apache.orc.CompressionKind;
 import org.slf4j.Logger;
@@ -62,6 +60,19 @@ public final class OrcCodecPool {
     return codec;
   }
 
+  public static void returnCodecSafely(
+      CompressionKind kind, CompressionCodec codec, boolean hasError) {
+    try {
+      if (!hasError) {
+        returnCodec(kind, codec);
+      } else {
+        codec.close();
+      }
+    } catch (Exception ex) {
+      LOG.error("Ignoring codec cleanup error", ex);
+    }
+  }
+
   public static void returnCodec(CompressionKind kind, CompressionCodec codec) {
     if (codec == null) {
       return;

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/OrcTail.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/OrcTail.java b/java/core/src/java/org/apache/orc/impl/OrcTail.java
index 3c78874..0136511 100644
--- a/java/core/src/java/org/apache/orc/impl/OrcTail.java
+++ b/java/core/src/java/org/apache/orc/impl/OrcTail.java
@@ -107,11 +107,13 @@ public final class OrcTail {
     if (serializedTail == null) return null;
     if (metadata == null) {
       CompressionCodec codec = OrcCodecPool.getCodec(getCompressionKind());
+      boolean isCodecError = true;
       try {
         metadata = extractMetadata(serializedTail, 0,
             (int) fileTail.getPostscript().getMetadataLength(), codec, getCompressionBufferSize());
+        isCodecError = false;
       } finally {
-        OrcCodecPool.returnCodec(getCompressionKind(), codec);
+        OrcCodecPool.returnCodecSafely(getCompressionKind(), codec, isCodecError);
       }
       // clear does not clear the contents but sets position to 0 and limit = capacity
       serializedTail.clear();

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
index 918fae8..5d7f75a 100644
--- a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
+++ b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
@@ -225,7 +225,12 @@ public class PhysicalFsWriter implements PhysicalWriter {
 
   @Override
   public void close() throws IOException {
-    OrcCodecPool.returnCodec(compress, codec);
+    // We don't use the codec directly but do give it out codec in getCompressionCodec;
+    // that is used in tests, for boolean checks, and in StreamFactory. Some of the changes that
+    // would get rid of this pattern require cross-project interface changes, so just return the
+    // codec for now. If the codec is broken, reset will usually throw, so this is still the\
+    // correct thing to do.
+    OrcCodecPool.returnCodecSafely(compress, codec, false);
     rawWriter.close();
   }
 

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/ReaderImpl.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/ReaderImpl.java b/java/core/src/java/org/apache/orc/impl/ReaderImpl.java
index 0daa0ed..94d693b 100644
--- a/java/core/src/java/org/apache/orc/impl/ReaderImpl.java
+++ b/java/core/src/java/org/apache/orc/impl/ReaderImpl.java
@@ -463,6 +463,7 @@ public class ReaderImpl implements Reader {
     CompressionKind kind = CompressionKind.valueOf(ps.getCompression().name());
     OrcProto.FileTail.Builder fileTailBuilder;
     CompressionCodec codec = OrcCodecPool.getCodec(kind);
+    boolean isCodecError = true;
     try {
       OrcProto.Footer footer = extractFooter(buffer,
         (int) (buffer.position() + ps.getMetadataLength()),
@@ -472,8 +473,9 @@ public class ReaderImpl implements Reader {
         .setPostscript(ps)
         .setFooter(footer)
         .setFileLength(fileLength);
+      isCodecError = false;
     } finally {
-      OrcCodecPool.returnCodec(kind, codec);
+      OrcCodecPool.returnCodecSafely(kind, codec, isCodecError);
     }
     // clear does not clear the contents but sets position to 0 and limit = capacity
     buffer.clear();
@@ -593,10 +595,12 @@ public class ReaderImpl implements Reader {
       buffer.reset();
       OrcProto.Footer footer;
       CompressionCodec codec = OrcCodecPool.getCodec(compressionKind);
+      boolean isCodecError = true;
       try {
         footer = extractFooter(footerBuffer, 0, footerSize, codec, bufferSize);
+        isCodecError = false;
       } finally {
-        OrcCodecPool.returnCodec(compressionKind, codec);
+        OrcCodecPool.returnCodecSafely(compressionKind, codec, isCodecError);
       }
       fileTailBuilder.setFooter(footer);
     }
@@ -782,10 +786,12 @@ public class ReaderImpl implements Reader {
   public List<StripeStatistics> getStripeStatistics() throws IOException {
     if (metadata == null) {
       CompressionCodec codec = OrcCodecPool.getCodec(compressionKind);
+      boolean isCodecError = true;
       try {
         metadata = extractMetadata(tail.getSerializedTail(), 0, metadataSize, codec, bufferSize);
+        isCodecError = false;
       } finally {
-        OrcCodecPool.returnCodec(compressionKind, codec);
+        OrcCodecPool.returnCodecSafely(compressionKind, codec, isCodecError);
       }
     }
     if (stripeStats == null) {

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java b/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
index 0dacc70..d7722d1 100644
--- a/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
+++ b/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
@@ -17,8 +17,9 @@
  */
 package org.apache.orc.impl;
 
-import org.apache.orc.CompressionKind;
+import com.google.common.annotations.VisibleForTesting;
 
+import org.apache.orc.CompressionKind;
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.sql.Date;
@@ -29,7 +30,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TimeZone;
-
 import org.apache.orc.OrcFile;
 import org.apache.orc.util.BloomFilter;
 import org.apache.orc.util.BloomFilterIO;
@@ -1353,6 +1353,8 @@ public class RecordReaderImpl implements RecordReader {
     return result;
   }
 
+  // TODO: remove this
+  @VisibleForTesting
   public CompressionCodec getCompressionCodec() {
     return dataReader.getCompressionCodec();
   }

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java b/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
index 1e2d0f1..b486571 100644
--- a/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
+++ b/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
@@ -149,6 +149,7 @@ public class RecordReaderUtils {
     private final Path path;
     private final boolean useZeroCopy;
     private final CompressionCodec codec;
+    private boolean hasCodecError = false;
     private final int bufferSize;
     private final int typeCount;
     private CompressionKind compressionKind;
@@ -172,6 +173,7 @@ public class RecordReaderUtils {
     public void open() throws IOException {
       this.file = fs.open(path);
       if (useZeroCopy) {
+        // ZCR only uses codec for boolean checks.
         zcr = RecordReaderUtils.createZeroCopyShim(file, codec, pool);
       } else {
         zcr = null;
@@ -228,11 +230,18 @@ public class RecordReaderUtils {
                 ByteBuffer bb = range.getData().duplicate();
                 bb.position((int) (offset - range.getOffset()));
                 bb.limit((int) (bb.position() + stream.getLength()));
-                indexes[column] = OrcProto.RowIndex.parseFrom(
-                    InStream.createCodedInputStream("index",
-                        ReaderImpl.singleton(new BufferChunk(bb, 0)),
-                        stream.getLength(),
-                    codec, bufferSize));
+                boolean isOk = false;
+                try {
+                  indexes[column] = OrcProto.RowIndex.parseFrom(
+                      InStream.createCodedInputStream("index",
+                          ReaderImpl.singleton(new BufferChunk(bb, 0)),
+                          stream.getLength(), codec, bufferSize));
+                  isOk = true;
+                } finally {
+                  if (!isOk) {
+                    hasCodecError = true;
+                  }
+                }
               }
               break;
             case BLOOM_FILTER:
@@ -241,10 +250,18 @@ public class RecordReaderUtils {
                 ByteBuffer bb = range.getData().duplicate();
                 bb.position((int) (offset - range.getOffset()));
                 bb.limit((int) (bb.position() + stream.getLength()));
-                bloomFilterIndices[column] = OrcProto.BloomFilterIndex.parseFrom
-                    (InStream.createCodedInputStream("bloom_filter",
-                        ReaderImpl.singleton(new BufferChunk(bb, 0)),
-                    stream.getLength(), codec, bufferSize));
+                boolean isOk = false;
+                try {
+                  bloomFilterIndices[column] = OrcProto.BloomFilterIndex.parseFrom
+                      (InStream.createCodedInputStream("bloom_filter",
+                          ReaderImpl.singleton(new BufferChunk(bb, 0)),
+                      stream.getLength(), codec, bufferSize));
+                  isOk = true;
+                } finally {
+                  if (!isOk) {
+                    hasCodecError = true;
+                  }
+                }
               }
               break;
             default:
@@ -267,9 +284,18 @@ public class RecordReaderUtils {
       // read the footer
       ByteBuffer tailBuf = ByteBuffer.allocate(tailLength);
       file.readFully(offset, tailBuf.array(), tailBuf.arrayOffset(), tailLength);
-      return OrcProto.StripeFooter.parseFrom(InStream.createCodedInputStream("footer",
-          ReaderImpl.singleton(new BufferChunk(tailBuf, 0)),
-          tailLength, codec, bufferSize));
+      boolean isOk = false;
+      try {
+        OrcProto.StripeFooter result = OrcProto.StripeFooter.parseFrom(
+            InStream.createCodedInputStream("footer", ReaderImpl.singleton(
+                new BufferChunk(tailBuf, 0)), tailLength, codec, bufferSize));
+        isOk = true;
+        return result;
+      } finally {
+        if (!isOk) {
+          hasCodecError = true;
+        }
+      }
     }
 
     @Override
@@ -281,7 +307,7 @@ public class RecordReaderUtils {
     @Override
     public void close() throws IOException {
       if (codec != null) {
-        OrcCodecPool.returnCodec(compressionKind, codec);
+        OrcCodecPool.returnCodecSafely(compressionKind, codec, hasCodecError);
       }
       if (pool != null) {
         pool.clear();
@@ -315,6 +341,8 @@ public class RecordReaderUtils {
 
     @Override
     public CompressionCodec getCompressionCodec() {
+      // Note: see comments in PhysicalFsWriter; we should probably get rid of this usage
+      //       pattern to make error handling for codec pool more robust.
       return codec;
     }
   }

http://git-wip-us.apache.org/repos/asf/orc/blob/d9727cb3/java/core/src/java/org/apache/orc/impl/WriterImpl.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/WriterImpl.java b/java/core/src/java/org/apache/orc/impl/WriterImpl.java
index 90b410c..343f565 100644
--- a/java/core/src/java/org/apache/orc/impl/WriterImpl.java
+++ b/java/core/src/java/org/apache/orc/impl/WriterImpl.java
@@ -18,6 +18,8 @@
 
 package org.apache.orc.impl;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -26,7 +28,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.TimeZone;
 import java.util.TreeMap;
-
 import io.airlift.compress.lz4.Lz4Compressor;
 import io.airlift.compress.lz4.Lz4Decompressor;
 import io.airlift.compress.lzo.LzoCompressor;
@@ -50,7 +51,6 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
-
 import com.google.protobuf.ByteString;
 
 /**
@@ -645,6 +645,8 @@ public class WriterImpl implements Writer, MemoryManager.Callback {
     return ReaderImpl.deserializeStats(builder.getStatisticsList());
   }
 
+  // TODO: remove this
+  @VisibleForTesting
   public CompressionCodec getCompressionCodec() {
     return physicalWriter.getCompressionCodec();
   }


[3/3] orc git commit: update

Posted by se...@apache.org.
update


Project: http://git-wip-us.apache.org/repos/asf/orc/repo
Commit: http://git-wip-us.apache.org/repos/asf/orc/commit/c6a58c67
Tree: http://git-wip-us.apache.org/repos/asf/orc/tree/c6a58c67
Diff: http://git-wip-us.apache.org/repos/asf/orc/diff/c6a58c67

Branch: refs/heads/orc-310
Commit: c6a58c67051b136246030153e798980200b47f1a
Parents: 06c93b3
Author: sergey <se...@apache.org>
Authored: Thu Mar 8 17:13:27 2018 -0800
Committer: sergey <se...@apache.org>
Committed: Thu Mar 8 17:13:27 2018 -0800

----------------------------------------------------------------------
 .../java/org/apache/orc/impl/OrcCodecPool.java  |  3 +++
 .../org/apache/orc/impl/PhysicalFsWriter.java   |  8 +++++---
 .../org/apache/orc/impl/RecordReaderUtils.java  | 21 ++++++++++++--------
 3 files changed, 21 insertions(+), 11 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/orc/blob/c6a58c67/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
index 4ae43a2..8269316 100644
--- a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
+++ b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
@@ -69,6 +69,9 @@ public final class OrcCodecPool {
    */
   public static void returnCodecSafely(
       CompressionKind kind, CompressionCodec codec, boolean observedError) {
+    if (codec == null) {
+      return;
+    }
     try {
       if (!observedError) {
         returnCodec(kind, codec);

http://git-wip-us.apache.org/repos/asf/orc/blob/c6a58c67/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
index 5d7f75a..3f8e4e2 100644
--- a/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
+++ b/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java
@@ -46,7 +46,7 @@ public class PhysicalFsWriter implements PhysicalWriter {
   private static final int HDFS_BUFFER_SIZE = 256 * 1024;
   private static final HadoopShims shims = HadoopShimsFactory.get();
 
-  private final FSDataOutputStream rawWriter;
+  private FSDataOutputStream rawWriter;
   // the compressed metadata information outStream
   private OutStream writer = null;
   // a protobuf outStream around streamFactory
@@ -58,7 +58,7 @@ public class PhysicalFsWriter implements PhysicalWriter {
   private final double paddingTolerance;
   private final long defaultStripeSize;
   private final CompressionKind compress;
-  private final CompressionCodec codec;
+  private CompressionCodec codec;
   private final boolean addBlockPadding;
 
   // the streams that make up the current stripe
@@ -228,10 +228,12 @@ public class PhysicalFsWriter implements PhysicalWriter {
     // We don't use the codec directly but do give it out codec in getCompressionCodec;
     // that is used in tests, for boolean checks, and in StreamFactory. Some of the changes that
     // would get rid of this pattern require cross-project interface changes, so just return the
-    // codec for now. If the codec is broken, reset will usually throw, so this is still the\
+    // codec for now. If the codec is broken, reset will usually throw, so this is still the
     // correct thing to do.
     OrcCodecPool.returnCodecSafely(compress, codec, false);
+    codec = null;
     rawWriter.close();
+    rawWriter = null;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/orc/blob/c6a58c67/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java b/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
index b486571..9d9e31a 100644
--- a/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
+++ b/java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
@@ -143,12 +143,12 @@ public class RecordReaderUtils {
 
   private static class DefaultDataReader implements DataReader {
     private FSDataInputStream file = null;
-    private final ByteBufferAllocatorPool pool;
+    private ByteBufferAllocatorPool pool;
     private HadoopShims.ZeroCopyReaderShim zcr = null;
     private final FileSystem fs;
     private final Path path;
     private final boolean useZeroCopy;
-    private final CompressionCodec codec;
+    private CompressionCodec codec;
     private boolean hasCodecError = false;
     private final int bufferSize;
     private final int typeCount;
@@ -162,11 +162,6 @@ public class RecordReaderUtils {
       this.codec = OrcCodecPool.getCodec(compressionKind);
       this.bufferSize = properties.getBufferSize();
       this.typeCount = properties.getTypeCount();
-      if (useZeroCopy) {
-        this.pool = new ByteBufferAllocatorPool();
-      } else {
-        this.pool = null;
-      }
     }
 
     @Override
@@ -174,6 +169,7 @@ public class RecordReaderUtils {
       this.file = fs.open(path);
       if (useZeroCopy) {
         // ZCR only uses codec for boolean checks.
+        pool = new ByteBufferAllocatorPool();
         zcr = RecordReaderUtils.createZeroCopyShim(file, codec, pool);
       } else {
         zcr = null;
@@ -308,6 +304,7 @@ public class RecordReaderUtils {
     public void close() throws IOException {
       if (codec != null) {
         OrcCodecPool.returnCodecSafely(compressionKind, codec, hasCodecError);
+        codec = null;
       }
       if (pool != null) {
         pool.clear();
@@ -316,6 +313,7 @@ public class RecordReaderUtils {
       try (HadoopShims.ZeroCopyReaderShim myZcr = zcr) {
         if (file != null) {
           file.close();
+          file = null;
         }
       }
     }
@@ -332,8 +330,15 @@ public class RecordReaderUtils {
 
     @Override
     public DataReader clone() {
+      if (this.file != null) {
+        throw new UnsupportedOperationException(
+            "Cannot clone a DataReader that is already opened");
+      }
       try {
-        return (DataReader) super.clone();
+        DefaultDataReader clone = (DefaultDataReader) super.clone();
+        // Make sure we don't share the same codec between two readers.
+        clone.codec = OrcCodecPool.getCodec(clone.compressionKind);
+        return clone;
       } catch (CloneNotSupportedException e) {
         throw new UnsupportedOperationException("uncloneable", e);
       }


[2/3] orc git commit: ORC-310 CR 1

Posted by se...@apache.org.
ORC-310 CR 1


Project: http://git-wip-us.apache.org/repos/asf/orc/repo
Commit: http://git-wip-us.apache.org/repos/asf/orc/commit/06c93b30
Tree: http://git-wip-us.apache.org/repos/asf/orc/tree/06c93b30
Diff: http://git-wip-us.apache.org/repos/asf/orc/diff/06c93b30

Branch: refs/heads/orc-310
Commit: 06c93b30b9b075969c984e0c60f783572c6a4a24
Parents: d9727cb
Author: sergey <se...@apache.org>
Authored: Thu Mar 1 15:41:45 2018 -0800
Committer: sergey <se...@apache.org>
Committed: Thu Mar 1 15:41:45 2018 -0800

----------------------------------------------------------------------
 .../src/java/org/apache/orc/impl/OrcCodecPool.java | 17 +++++++++++++++--
 .../java/org/apache/orc/impl/RecordReaderImpl.java |  5 +----
 .../src/java/org/apache/orc/impl/WriterImpl.java   |  5 +----
 3 files changed, 17 insertions(+), 10 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/orc/blob/06c93b30/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
index 34f0bd4..4ae43a2 100644
--- a/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
+++ b/java/core/src/java/org/apache/orc/impl/OrcCodecPool.java
@@ -60,10 +60,17 @@ public final class OrcCodecPool {
     return codec;
   }
 
+  /**
+   * Returns the codec to the pool or closes it, suppressing exceptions.
+   * @param kind Compression kind.
+   * @param codec Codec.
+   * @param observedError Whether the caller has definitely observed an error with the codec.
+   *                      The pool will also do basic correctness check on the codec itself.
+   */
   public static void returnCodecSafely(
-      CompressionKind kind, CompressionCodec codec, boolean hasError) {
+      CompressionKind kind, CompressionCodec codec, boolean observedError) {
     try {
-      if (!hasError) {
+      if (!observedError) {
         returnCodec(kind, codec);
       } else {
         codec.close();
@@ -73,6 +80,12 @@ public final class OrcCodecPool {
     }
   }
 
+  /**
+   * Returns the codec to the pool, potentially failing if the codec cannot be reused, or if
+   * the codec is unneeded and cannot be closed.
+   * @param kind Compression kind.
+   * @param codec Codec.
+   */
   public static void returnCodec(CompressionKind kind, CompressionCodec codec) {
     if (codec == null) {
       return;

http://git-wip-us.apache.org/repos/asf/orc/blob/06c93b30/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java b/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
index d7722d1..ff8dfd7 100644
--- a/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
+++ b/java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
@@ -17,8 +17,6 @@
  */
 package org.apache.orc.impl;
 
-import com.google.common.annotations.VisibleForTesting;
-
 import org.apache.orc.CompressionKind;
 import java.io.IOException;
 import java.math.BigDecimal;
@@ -1353,8 +1351,7 @@ public class RecordReaderImpl implements RecordReader {
     return result;
   }
 
-  // TODO: remove this
-  @VisibleForTesting
+  // TODO: pollutes the interface for tests only - we should remove this.
   public CompressionCodec getCompressionCodec() {
     return dataReader.getCompressionCodec();
   }

http://git-wip-us.apache.org/repos/asf/orc/blob/06c93b30/java/core/src/java/org/apache/orc/impl/WriterImpl.java
----------------------------------------------------------------------
diff --git a/java/core/src/java/org/apache/orc/impl/WriterImpl.java b/java/core/src/java/org/apache/orc/impl/WriterImpl.java
index 343f565..0186445 100644
--- a/java/core/src/java/org/apache/orc/impl/WriterImpl.java
+++ b/java/core/src/java/org/apache/orc/impl/WriterImpl.java
@@ -18,8 +18,6 @@
 
 package org.apache.orc.impl;
 
-import com.google.common.annotations.VisibleForTesting;
-
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -645,8 +643,7 @@ public class WriterImpl implements Writer, MemoryManager.Callback {
     return ReaderImpl.deserializeStats(builder.getStatisticsList());
   }
 
-  // TODO: remove this
-  @VisibleForTesting
+  // TODO: pollutes the interface for tests only - we should remove this.
   public CompressionCodec getCompressionCodec() {
     return physicalWriter.getCompressionCodec();
   }