----------------------------------------------------------- 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 > > > Addressed review comments from Jacques > > > DRILL-256 revised patch > > > Diffs > ----- > > distribution/src/resources/drill-override.conf > c2ed9df26b63ad81b06aa665af765e67ec930656 > exec/java-exec/pom.xml 063e60e038beb2c3100848985afea9c651af9ca6 > exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java > 72776d1a79cdb58241a9de72cb985462b3579068 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/DrillSerializable.java > 534d78106fd61ab16a428e6d46bced563724fb15 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HCDrillSerializableWrapper.java > 3f2c41c5e61fd097a697b6c31a3de20b307d1b50 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HCSerializableWrapperClasses.java > d22723ab021259197c8fa4f2cc23270301990de4 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HCVectorAccessibleSerializer.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/cache/HazelCache.java > 577dfebb1e9108650ea72b00f844748de3de56fb > exec/java-exec/src/main/java/org/apache/drill/exec/cache/LocalCache.java > 7ad6ec687e02a90e4936bcae16e8ef666818c327 > exec/java-exec/src/main/java/org/apache/drill/exec/cache/ProtoBufWrap.java > 4aea645afd278a6b0233eff19487dbd66c6a0272 > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorContainerSerializable.java > 1e6eeacec76ebd434720849b669735c9cf82df05 > > exec/java-exec/src/main/java/org/apache/drill/exec/client/QuerySubmitter.java > 160ef7ff21b079cd0e51aa5b62af09c96e932b5f > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > 352c467a8f26183631f03a89c5cf21c1df18b3ac > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java > bf6c68ca2d67d8d40001ee21656345ceb823ae75 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java > 5692b9f0827eec39671a29cee064e8247b7d344a > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Trace.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java > 844b3a750b4fd4bd510b0ada1a4724f127ab6b12 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TraceInjector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java > 7dc7d559dfbd5af4a607a4ff9a5ff8b0d59c0e93 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceBatchCreator.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/trace/TraceRecordBatch.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractSingleRecordBatch.java > 25284b698a5f20e8ef18a7fdc0a352b9e5b170d9 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java > 4057c58155afa62443ed36a5070032f75e14ee15 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java > 5fb9b0a65aa7b5b9f5950b87423d3ff17d62793e > > exec/java-exec/src/main/java/org/apache/drill/exec/record/selection/SelectionVector2.java > 6105eada605304bd7655c5cc9142efdd65ac02f1 > exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java > 052f4155db5f55f87224f46bc250cf69efd9d793 > exec/java-exec/src/main/resources/drill-module.conf > 7e40a05c4a8194b330c86ac2caa4b764327b0e23 > > exec/java-exec/src/test/java/org/apache/drill/exec/cache/TestVectorCache.java > 39ec7207b9db35092d70cec435e1458bbfda77dd > > exec/java-exec/src/test/java/org/apache/drill/exec/cache/TestWriteToDisk.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/trace/TestTraceMultiRecordBatch.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/trace/TestTraceOutputDump.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestLoad.java > 94d66d4c2d10a848b4bdfafb13027b54564b9e68 > exec/java-exec/src/test/resources/trace/multi_record_batch_trace.json > PRE-CREATION > exec/java-exec/src/test/resources/trace/simple_trace.json PRE-CREATION > protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java > faf67cd33dca1f163640c06608dda96cc674ba27 > protocol/src/main/protobuf/UserBitShared.proto > 7e2506c712100d278a9b27a16dae582188d62bed > > Diff: https://reviews.apache.org/r/15111/diff/ > > > Testing > ------- > > > Thanks, > > Steven Phillips > >
