You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by sershe <gi...@git.apache.org> on 2018/03/01 00:01:37 UTC
[GitHub] orc pull request #222: ORC-310 better error handling for codec
GitHub user sershe opened a pull request:
https://github.com/apache/orc/pull/222
ORC-310 better error handling for codec
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sershe/orc orc-310
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/orc/pull/222.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #222
----
commit d9727cb36c9b1e9f1c045e29d928401a680062cb
Author: sergey <se...@...>
Date: 2018-03-01T00:00:51Z
ORC-310
better error handling for codec
----
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by sershe <gi...@git.apache.org>.
Github user sershe commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r171728816
--- Diff: java/core/src/java/org/apache/orc/impl/WriterImpl.java ---
@@ -645,6 +645,8 @@ public void appendUserMetadata(List<OrcProto.UserMetadataItem> userMetadata) {
return ReaderImpl.deserializeStats(builder.getStatisticsList());
}
+ // TODO: remove this
--- End diff --
Removed the attribute, clarified the TODO
---
[GitHub] orc issue #222: ORC-310 better error handling for codec
Posted by sershe <gi...@git.apache.org>.
Github user sershe commented on the issue:
https://github.com/apache/orc/pull/222
Updated the JIRA with motivation.
I will rename the flag; there isn't a better way primarily because of the TODOs you are asking me to remove - we don't have access to information about errors if the user and the return-er of the codec are two different pieces of code at different times.
There's also no good way to check the state of the codec to see if it's valid or not. Hadoop ZLib codec may just throw NPE in reset when it's invalid, so the "Safely" catch block will handle that case. However it's nice to give user an opportunity to be proactive about this, esp. in the case of the get-use-return usage pattern
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r171720713
--- Diff: java/core/src/java/org/apache/orc/impl/ReaderImpl.java ---
@@ -472,8 +473,9 @@ public static OrcTail extractFileTail(ByteBuffer buffer, long fileLength, long m
.setPostscript(ps)
.setFooter(footer)
.setFileLength(fileLength);
+ isCodecError = false;
} finally {
--- End diff --
try {} finally is pretty broken. You are better off using the try-with-resources statement.
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by sershe <gi...@git.apache.org>.
Github user sershe commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r173943752
--- Diff: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java ---
@@ -306,15 +330,24 @@ public void releaseBuffer(ByteBuffer buffer) {
@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);
--- End diff --
super.clone() is Object.clone(), so no
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/orc/pull/222
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r171720836
--- Diff: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java ---
@@ -1353,6 +1353,8 @@ public static String encodeTranslatedSargColumn(int rootColumn, Integer indexInS
return result;
}
+ // TODO: remove this
--- End diff --
Revert the changes in this file.
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by prasanthj <gi...@git.apache.org>.
Github user prasanthj commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r173941203
--- Diff: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java ---
@@ -306,15 +330,24 @@ public void releaseBuffer(ByteBuffer buffer) {
@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);
--- End diff --
can this be moved inside super.clone()?
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r171719585
--- Diff: java/core/src/java/org/apache/orc/impl/WriterImpl.java ---
@@ -645,6 +645,8 @@ public void appendUserMetadata(List<OrcProto.UserMetadataItem> userMetadata) {
return ReaderImpl.deserializeStats(builder.getStatisticsList());
}
+ // TODO: remove this
--- End diff --
Remove the changes in this file. We do not want a guice dependency.
---
[GitHub] orc pull request #222: ORC-310 better error handling for codec
Posted by sershe <gi...@git.apache.org>.
Github user sershe commented on a diff in the pull request:
https://github.com/apache/orc/pull/222#discussion_r171728763
--- Diff: java/core/src/java/org/apache/orc/impl/ReaderImpl.java ---
@@ -472,8 +473,9 @@ public static OrcTail extractFileTail(ByteBuffer buffer, long fileLength, long m
.setPostscript(ps)
.setFooter(footer)
.setFileLength(fileLength);
+ isCodecError = false;
} finally {
--- End diff --
existing code
---