pig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Julien Le Dem" <jul...@ledem.net>
Subject Re: Review Request 14552: PIG-3480 TFile-based tmpfile compression crashes in some cases
Date Thu, 10 Oct 2013 20:50:31 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message