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

Switch to Threaded View
Drill, mail # dev - Review Request 15111: Patch for DRILL-271


Copy link to this message
-
Re: Review Request 15111: Patch for DRILL-271
Mehant Baid 2013-10-31, 07:12

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15111/#review27859
-----------------------------------------------------------
I reviewed most part of the trace operator. Looks good to me, I had a few minor comments.
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54261>

    Can we get rid of this? Are you planning to use it somewhere?
    

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54260>

    Do you think it'd be good to chain the constructors. It'll be easier to change something in the future.

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54288>

    Could you add minor comments describing the purpose of this function, even though its easy to understand, it'd be good to have a couple of lines.
    
    Also the same for this class.

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54263>

    Instead of hardcoding, you can use SelectionVector2.RECORD_SIZE

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54280>

    You could get rid of the svBuf.writerIndex(). Instead in the svBuf.getBytes() you could directly use the length to be the selection vector's size.
    
    I was using it slightly differently so I needed svBuf's writer index to be set. But getBytes() does not use the writer index so you can pass in the length directly.

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54285>

    I am not sure if its a good idea to combine ByteBuf memory management and its serialization into one class. Seems to me it'd be the responsibility of the client of this class to release or retain the buffers.

exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
<https://reviews.apache.org/r/15111/#comment54276>

    Do you think it would be a good idea to move this logic in a separate utility class? I am thinking VectorAccessibleSerializable should contain code to write ByteBuf's to OutputStream and read it from InputStream.
    
    However I don't feel too strongly about this, I defer it to your judgement.

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceRecordBatch.java
<https://reviews.apache.org/r/15111/#comment54249>

    Can you remove the import statements that are no longer in use, since most of the logic has moved.

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceRecordBatch.java
<https://reviews.apache.org/r/15111/#comment54250>

    Comment is out of sync. Could you update the comment?
- Mehant Baid
On Oct. 31, 2013, 2:56 a.m., Steven Phillips wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15111/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2013, 2:56 a.m.)
>
>
> Review request for drill.
>
>
> Bugs: DRILL-271
>     https://issues.apache.org/jira/browse/DRILL-271
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> fix filename bug in TestTraceOutputDump
>
>
> abstract out serialization in trace record batch by using VectorAccessibleSerializable. Add ability to retain vectors.
>
>
> release buffer in VectorAccessibleSerializable read. Also read directly from stream.
>
>
> refactor DistributedCache code. Uses hazel cast 3.1 and custom serialization.
>
>
> minor fix in QuerySubmitter
>
>
> retool VectorContainerSerializable to work with containers and batches. also modify DrillSerializable to work with InputStream, OutputStream