Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Pig >> mail # dev >> Review Request 14552: PIG-3480 TFile-based tmpfile compression crashes in some cases


+
Aniket Mokashi 2013-10-09, 00:43
+
Aniket Mokashi 2013-10-09, 01:02
+
Aniket Mokashi 2013-10-09, 01:32
+
Aniket Mokashi 2013-10-09, 18:00
Copy link to this message
-
Re: Review Request 14552: PIG-3480 TFile-based tmpfile compression crashes in some cases

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14552/#review26894
-----------------------------------------------------------
Overall this looks good to me.
Some comments regarding error handling in conf and defaults.
trunk/src/org/apache/pig/backend/hadoop/executionengine/util/MapRedUtil.java
<https://reviews.apache.org/r/14552/#comment52333>

    can we fix indentation and whitespace at the same time?
    - curly at end of line
    - spaces around operators
    - whitespace at end of line

trunk/src/org/apache/pig/impl/util/Utils.java
<https://reviews.apache.org/r/14552/#comment52335>

    We should throw an exception if the trimmed lowercased  string is something we don't expect.
    otherwise typos (seqFile, sequencefile, ...) will lead to unexpected behavior.
    
    the call becomes then:
    ...getProperty(...PIG_TEMP_FILE_COMPRESSION_STORAGE, "tfile");
    
    example message:
    PigConfiguration.PIG_TEMP_FILE_COMPRESSION_STORAGE +": expected storage seqfile or tfile (default tfile)"
    
    

trunk/src/org/apache/pig/impl/util/Utils.java
<https://reviews.apache.org/r/14552/#comment52336>

    duplicate logic?

trunk/src/org/apache/pig/impl/util/Utils.java
<https://reviews.apache.org/r/14552/#comment52339>

    you can use constants or enums for the available codecs.
    they could have fields for the corresponding codec class name and whether tfile supports them.

trunk/src/org/apache/pig/impl/util/Utils.java
<https://reviews.apache.org/r/14552/#comment52337>

    this is not consistent with the previous lines:
    else if (codec.equals("")) {
    ...
    } else {
    ...
    }

trunk/src/org/apache/pig/impl/util/Utils.java
<https://reviews.apache.org/r/14552/#comment52338>

    else throw exception?
    we have a default if it's not set but we don't do anything if it's an invalid compression codec
- Julien Le Dem
On Oct. 9, 2013, 6 p.m., Aniket Mokashi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14552/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 6 p.m.)
>
>
> Review request for pig, Cheolsoo Park, Dmitriy Ryaboy, Julien Le Dem, and Rohini Palaniswamy.
>
>
> Bugs: PIG-3480
>     https://issues.apache.org/jira/browse/PIG-3480
>
>
> Repository: pig
>
>
> Description
> -------
>
> - Added a new parameter to make SequenceFileInterStorage optional.
> - Added tests
> - Refactored apis
>
>
> Diffs
> -----
>
>   trunk/conf/pig.properties 1530468
>   trunk/src/org/apache/pig/PigConfiguration.java 1530468
>   trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/JobControlCompiler.java 1530468
>   trunk/src/org/apache/pig/backend/hadoop/executionengine/mapReduceLayer/partitioners/WeightedRangePartitioner.java 1530468
>   trunk/src/org/apache/pig/backend/hadoop/executionengine/physicalLayer/relationalOperators/POPartitionRearrange.java 1530468
>   trunk/src/org/apache/pig/backend/hadoop/executionengine/util/MapRedUtil.java 1530468
>   trunk/src/org/apache/pig/impl/io/InterStorage.java 1530468
>   trunk/src/org/apache/pig/impl/io/SequenceFileInterStorage.java PRE-CREATION
>   trunk/src/org/apache/pig/impl/io/TFileStorage.java 1530468
>   trunk/src/org/apache/pig/impl/util/Utils.java 1530468
>   trunk/test/org/apache/pig/test/TestTmpFileCompression.java 1530468
>
> Diff: https://reviews.apache.org/r/14552/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aniket Mokashi
>
>

+
Aniket Mokashi 2013-10-10, 22:06
+
Julien Le Dem 2013-10-11, 04:57
+
Aniket Mokashi 2013-10-11, 04:31
+
Julien Le Dem 2013-10-11, 03:51
+
Aniket Mokashi 2013-10-11, 20:43
+
Dmitriy Ryaboy 2013-10-11, 23:40
+
Aniket Mokashi 2013-10-12, 00:28
+
Aniket Mokashi 2013-10-12, 00:44
+
Julien Le Dem 2013-10-13, 11:40
+
Aniket Mokashi 2013-10-14, 03:21
+
Aniket Mokashi 2013-10-09, 01:10