You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jacques Nadeau <ja...@gmail.com> on 2015/05/06 15:22:53 UTC
Review Request 33893: DRILL-2959: Ensure release and management of
compression codecs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33893/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche and Steven Phillips.
Repository: drill-git
Description
-------
Create a DirectCodecFactory and DirectCodecPool which consolidate our compression tools use.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecPool.java PRE-CREATION
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetDirectByteBufferAllocator.java 45a1dc6
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetFormatPlugin.java cfa4c93
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java 3506ffa
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java d5586ce
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/PageReader.java 6a41a04
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 11d0042
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java 921d134
exec/java-exec/src/main/java/parquet/hadoop/CodecFactoryExposer.java 5438660
exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkIncReadStore.java 242cd28
exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkPageWriteStoreExposer.java 0e9dec0
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestDirectCodecFactory.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java e50e3fb
pom.xml f0f4bc5
Diff: https://reviews.apache.org/r/33893/diff/
Testing
-------
regression/unit in progress.
Thanks,
Jacques Nadeau
Re: Review Request 33893: DRILL-2959: Ensure release and management of
compression codecs.
Posted by Jacques Nadeau <ja...@gmail.com>.
> On May 6, 2015, 6:33 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java, line 51
> > <https://reviews.apache.org/r/33893/diff/1/?file=951150#file951150line51>
> >
> > what is the purpose of allocatedBuffers ? it looks like we are storing buffer references in it's key set, but we never actually use them.
nice catch. earlier version managed buffer release this way, now rely on compressors/decompressors to manage during release.
> On May 6, 2015, 6:33 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java, line 122
> > <https://reviews.apache.org/r/33893/diff/1/?file=951150#file951150line122>
> >
> > is this class needed anymore ?
keeping it but adding comment as to why we have it there.
> On May 6, 2015, 6:33 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java, line 113
> > <https://reviews.apache.org/r/33893/diff/1/?file=951150#file951150line113>
> >
> > same
yes
> On May 6, 2015, 6:33 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java, line 104
> > <https://reviews.apache.org/r/33893/diff/1/?file=951150#file951150line104>
> >
> > remove this comment ?
adding comment describing purpose
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33893/#review82685
-----------------------------------------------------------
On May 6, 2015, 1:22 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33893/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 1:22 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Create a DirectCodecFactory and DirectCodecPool which consolidate our compression tools use.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecPool.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetDirectByteBufferAllocator.java 45a1dc6
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetFormatPlugin.java cfa4c93
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java 3506ffa
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java d5586ce
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/PageReader.java 6a41a04
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 11d0042
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java 921d134
> exec/java-exec/src/main/java/parquet/hadoop/CodecFactoryExposer.java 5438660
> exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkIncReadStore.java 242cd28
> exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkPageWriteStoreExposer.java 0e9dec0
> exec/java-exec/src/test/java/org/apache/drill/exec/store/TestDirectCodecFactory.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java e50e3fb
> pom.xml f0f4bc5
>
> Diff: https://reviews.apache.org/r/33893/diff/
>
>
> Testing
> -------
>
> regression/unit in progress.
>
>
> Thanks,
>
> Jacques Nadeau
>
>
Re: Review Request 33893: DRILL-2959: Ensure release and management of
compression codecs.
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33893/#review82685
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java
<https://reviews.apache.org/r/33893/#comment133472>
what is the purpose of allocatedBuffers ? it looks like we are storing buffer references in it's key set, but we never actually use them.
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java
<https://reviews.apache.org/r/33893/#comment133473>
remove this comment ?
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java
<https://reviews.apache.org/r/33893/#comment133474>
same
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java
<https://reviews.apache.org/r/33893/#comment133475>
is this class needed anymore ?
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetDirectByteBufferAllocator.java
<https://reviews.apache.org/r/33893/#comment133471>
current implementations of Allocator will return null if it can't allocate the byte buffer. This will cause a NullPointerException in the next line.
We should check if bb is null and throw an OutOfMemoryRuntimeException instead
- abdelhakim deneche
On May 6, 2015, 1:22 p.m., Jacques Nadeau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33893/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 1:22 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Steven Phillips.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Create a DirectCodecFactory and DirectCodecPool which consolidate our compression tools use.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecFactory.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/DirectCodecPool.java PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetDirectByteBufferAllocator.java 45a1dc6
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetFormatPlugin.java cfa4c93
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java 3506ffa
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java d5586ce
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/PageReader.java 6a41a04
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java 11d0042
> exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java 921d134
> exec/java-exec/src/main/java/parquet/hadoop/CodecFactoryExposer.java 5438660
> exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkIncReadStore.java 242cd28
> exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkPageWriteStoreExposer.java 0e9dec0
> exec/java-exec/src/test/java/org/apache/drill/exec/store/TestDirectCodecFactory.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java e50e3fb
> pom.xml f0f4bc5
>
> Diff: https://reviews.apache.org/r/33893/diff/
>
>
> Testing
> -------
>
> regression/unit in progress.
>
>
> Thanks,
>
> Jacques Nadeau
>
>