-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-17945. JsonSerialization raises EOFException reading JSON data stored on google GCS #3501
HADOOP-17945. JsonSerialization raises EOFException reading JSON data stored on google GCS #3501
Conversation
… stored on google GCS (lifted from MAPREDUCE-7341) Change-Id: I0170f311cf0a33806c21c3c50e5e797fe16b0dbf
🎊 +1 overall
This message was automatically generated. |
@mehakmeet & @mukund-thakur could you take a quick look @ this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few questions.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JsonSerialization.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JsonSerialization.java
Show resolved
Hide resolved
@@ -181,5 +197,23 @@ public void testFileSystemEmptyPath() throws Throwable { | |||
} | |||
} | |||
|
|||
/** | |||
* 0 byte file through the load(path, status) API will fail with an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a test with 0 byte file but not passing the fileStatus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably
Just one test pending here then. Post that we can merge. |
i'm not in a setup to write tests. i don't have a work laptop and having just tried to check it out on my windows laptop, the paths for yarn timeline server are too long for the windows FS I'm not in a position to even check out the source, let alone compile it. can we take it as is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
… stored on google GCS (#3501) Contributed By: Steve Loughran
… stored on google GCS (apache#3501) Contributed By: Steve Loughran
(lifted from MAPREDUCE-7341)
How was this patch tested?