Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-18 Thread abdelhakim deneche
a loop. This should ideally be done once for a record batch otherwise the performance will be poor. abdelhakim deneche wrote: when aggregated all peer rows we may need to process multiple batches, setupEvaluatePeer() will be called once per batch in order for evaluatePeer() to read

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-18 Thread abdelhakim deneche
/window/lval.pby.oby.sql PRE-CREATION exec/java-exec/src/test/resources/window/ntile.sql PRE-CREATION Diff: https://reviews.apache.org/r/37482/diff/ Testing (updated) --- unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-17 Thread abdelhakim deneche
--- Thanks, abdelhakim deneche

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-17 Thread abdelhakim deneche
. To reply, visit: https://reviews.apache.org/r/37482/#review95521 --- On Aug. 14, 2015, 6:40 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-17 Thread abdelhakim deneche
PRE-CREATION exec/java-exec/src/test/resources/window/lval.pby.oby.sql PRE-CREATION exec/java-exec/src/test/resources/window/ntile.sql PRE-CREATION Diff: https://reviews.apache.org/r/37482/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-17 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37482/#review95533 --- On Aug. 17, 2015, 3:54 p.m., abdelhakim deneche wrote

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-14 Thread abdelhakim deneche
/window/lval.alltypes.sql PRE-CREATION exec/java-exec/src/test/resources/window/lval.pby.oby.sql PRE-CREATION exec/java-exec/src/test/resources/window/ntile.sql PRE-CREATION Diff: https://reviews.apache.org/r/37482/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions

2015-08-14 Thread abdelhakim deneche
-CREATION exec/java-exec/src/test/resources/window/lval.pby.oby.sql PRE-CREATION exec/java-exec/src/test/resources/window/ntile.sql PRE-CREATION Diff: https://reviews.apache.org/r/37482/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak memory if query is canceled before batches in rawBatches were loaded

2015-07-10 Thread abdelhakim deneche
3ca11f1 Diff: https://reviews.apache.org/r/34374/diff/ Testing --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 36233: Patch for DRILL-3202

2015-07-08 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36233/#review90961 --- LGTM +1 (non-binding) - abdelhakim deneche On July 7, 2015, 12

Re: Review Request 36222: Patch for DRILL-3393

2015-07-07 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36222/#review90715 --- +1, there is one small comment about unit test name - abdelhakim

Re: Review Request 36208: DRILL-3455: If a drillbit, that contains fragments for the current query, dies the QueryManager will fail the query even if those fragments already finished successfully

2015-07-06 Thread abdelhakim deneche
/QueryManager.java (line 534) https://reviews.apache.org/r/36208/#comment143638 you should probably include your fix for DRILL-3448 as part of this patch, it's 3 lines change - abdelhakim deneche On July 6, 2015, 5:10 p.m., Sudheesh Katkam wrote

Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns wrong results

2015-07-06 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36219/#review90577 --- +1 (non binding) - abdelhakim deneche On July 6, 2015, 9:23 p.m

Re: Review Request 36208: DRILL-3455: If a drillbit, that contains fragments for the current query, dies the QueryManager will fail the query even if those fragments already finished successfully

2015-07-06 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36208/#review90541 --- +1 (non-binding) - abdelhakim deneche On July 6, 2015, 6:47 p.m

Re: Review Request 36168: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-06 Thread abdelhakim deneche
://reviews.apache.org/r/36168/#review90514 --- On July 3, 2015, 4:13 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36168

Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns wrong results

2015-07-06 Thread abdelhakim deneche
/org/apache/drill/exec/planner/physical/WindowPrel.java (line 131) https://reviews.apache.org/r/36219/#comment143667 should we remove this block ? - abdelhakim deneche On July 6, 2015, 8:33 p.m., Sean Hsuan-Yi Chu wrote

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
/java/org/apache/drill/exec/memory/TestAllocators.java 74ce225 Diff: https://reviews.apache.org/r/36103/diff/ Testing (updated) --- testing... Thanks, abdelhakim deneche

Review Request 36168: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-03 Thread abdelhakim deneche
--- unit tests are passing, ongoing cluster testing... Thanks, abdelhakim deneche

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36103/ --- (Updated July 3, 2015, 2:37 p.m.) Review request

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java 74ce225 Diff: https://reviews.apache.org/r/36103/diff/ Testing --- testing... Thanks, abdelhakim deneche

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
at 10mb and just asked for a 50mb allocation. abdelhakim deneche wrote: I'm still not sure how to find which limit we did hit, at least with the current allocator abdelhakim deneche wrote: @hanifi, when I renamed allocateNew - allocateNewSafe I noticed 2 places where the code

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
at 10mb and just asked for a 50mb allocation. abdelhakim deneche wrote: I'm still not sure how to find which limit we did hit, at least with the current allocator @hanifi, when I renamed allocateNew - allocateNewSafe I noticed 2 places where the code doesn't check if the allocation

Re: Review Request 36168: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-03 Thread abdelhakim deneche
://reviews.apache.org/r/36168/diff/ Testing (updated) --- unit tests are passing along with customer and tpch100 Thanks, abdelhakim deneche

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-03 Thread abdelhakim deneche
-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java 74ce225 Diff: https://reviews.apache.org/r/36103/diff/ Testing (updated) --- all unit tests are passing along with customer and tpch100 Thanks, abdelhakim deneche

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-02 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34603/#review90193 --- On June 30, 2015, 10:37 p.m., abdelhakim deneche wrote

Re: Review Request 35887: DRILL-3312: PageReader.allocatePageData() calls BufferAllocator.buffer(int) but doesn't check if the result is null

2015-07-02 Thread abdelhakim deneche
--- On June 25, 2015, 7:38 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35887

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-02 Thread abdelhakim deneche
://reviews.apache.org/r/34603/#review90058 --- On June 30, 2015, 10:37 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-07-02 Thread abdelhakim deneche
generated e-mail. To reply, visit: https://reviews.apache.org/r/34603/#review90175 --- On June 30, 2015, 10:37 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-02 Thread abdelhakim deneche
://reviews.apache.org/r/36103/#review90137 --- On July 1, 2015, 9:21 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36103

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-01 Thread abdelhakim deneche
Thanks, abdelhakim deneche

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-01 Thread abdelhakim deneche
/r/36103/#review90134 --- On July 1, 2015, 9:21 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36103

Re: Review Request 36103: DRILL-3445: BufferAllocator.buffer() implementations should throw an OutOfMemoryRuntimeException

2015-07-01 Thread abdelhakim deneche
--- On July 1, 2015, 9:21 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36103

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
/QueryManager.java (line 286) https://reviews.apache.org/r/34603/#comment142841 when we fail to cancel a fragment we update it's status to FAILED. I guess we could introduce a new fragment state but thought that would be just too much for this specific use case - abdelhakim deneche On June

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
/exec/proto/beans/QueryResult.java 474e330 protocol/src/main/protobuf/UserBitShared.proto 0451fd2 Diff: https://reviews.apache.org/r/34603/diff/ Testing (updated) --- unit tests are passing Thanks, abdelhakim deneche

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
--- On June 30, 2015, 6:37 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34603/ --- (Updated June 30

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
to test for ? - abdelhakim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34603/#review89946 --- On June 30, 2015, 6:37 p.m., abdelhakim

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34603/#review89946 --- On June 30, 2015, 6:37 p.m., abdelhakim deneche wrote

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
a fair assumption for the fragment? abdelhakim deneche wrote: I didn't realize fragmentDone() also updates the fragment status. I will update the patch to fix that. I updated CancelMessageHandler.cancelFragment() to return OK if the fragment we are trying to cancel already finished

Review Request 36070: DRILL-3243: Need a better error message - Use of alias in window function definition

2015-06-30 Thread abdelhakim deneche
/apache/drill/exec/store/text/TestNewTextReader.java 76674f9 Diff: https://reviews.apache.org/r/36070/diff/ Testing --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
a fair assumption for the fragment? abdelhakim deneche wrote: I didn't realize fragmentDone() also updates the fragment status. I will update the patch to fix that. abdelhakim deneche wrote: I updated CancelMessageHandler.cancelFragment() to return OK if the fragment we

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-06-30 Thread abdelhakim deneche
--- unit tests are passing Thanks, abdelhakim deneche

Review Request 35887: DRILL-3312: PageReader.allocatePageData() calls BufferAllocator.buffer(int) but doesn't check if the result is null

2015-06-25 Thread abdelhakim deneche
: https://reviews.apache.org/r/35887/diff/ Testing --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 35609: DRILL-3243: Need a better error message - Use of alias in window function definition

2015-06-25 Thread abdelhakim deneche
76674f9 Diff: https://reviews.apache.org/r/35609/diff/ Testing --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Review Request 35881: DRILL-3354: TestBuilder can check if the number of result batches equals some expected value

2015-06-25 Thread abdelhakim deneche
/35881/diff/ Testing --- unit tests are passing Thanks, abdelhakim deneche

Review Request 35845: DRILL-3345: TestWindowFrame fails to properly check cases involving multiple batches

2015-06-24 Thread abdelhakim deneche
, abdelhakim deneche

Re: Review Request 35413: DRILL-3285: Part 1--Prep., Hygiene: Mainly, adding comments.

2015-06-17 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35413/#review88299 --- +1 (non binding) - abdelhakim deneche On June 17, 2015, 5:06 p.m

Re: Review Request 35413: DRILL-3285: Part 1--Prep., Hygiene: Mainly, adding comments.

2015-06-17 Thread abdelhakim deneche
.* (DRILL-3241) - abdelhakim deneche On June 17, 2015, 5:06 p.m., Daniel Barclay wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35413

Re: Review Request 35415: DRILL-3285: Part 3--Invert beforeFirstBatch - ! afterFirstBatch.

2015-06-17 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35415/#review88303 --- +1 (non-binding) - abdelhakim deneche On June 17, 2015, 5:06 p.m

Re: Review Request 35417: DRILL-3285: Part 5--Split old hacky next() into separate methods.

2015-06-17 Thread abdelhakim deneche
to the assertions, otherwise we'll get an empty AssertionError message in the logs exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillCursor.java (line 270) https://reviews.apache.org/r/35417/#comment140750 same here - abdelhakim deneche On June 17, 2015, 5:06 p.m., Daniel Barclay wrote

Re: Review Request 35417: DRILL-3285: Part 5--Split old hacky next() into separate methods.

2015-06-17 Thread abdelhakim deneche
- abdelhakim deneche On June 17, 2015, 5:06 p.m., Daniel Barclay wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35417/ --- (Updated

Review Request 35573: DRILL-3304: improve org.apache.drill.exec.expr.TypeHelper error messages when UnsupportedOprationException is thrown

2015-06-17 Thread abdelhakim deneche
/exceptions/ErrorHelper.java 5dd9b67 exec/java-exec/src/main/codegen/templates/TypeHelper.java ad818bd Diff: https://reviews.apache.org/r/35573/diff/ Testing --- all unit tests are passing along with functional/tpch100 Thanks, abdelhakim deneche

Re: Review Request 35393: DRILL-3220: IOB Exception when using constants in window functions

2015-06-15 Thread abdelhakim deneche
indeed - abdelhakim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35393/#review87899 --- On June 12, 2015, 5:40 p.m., abdelhakim deneche

Re: Review Request 35477: DRILL-3268: queries with empty OVER() clause return empty result set

2015-06-15 Thread abdelhakim deneche
: https://reviews.apache.org/r/35477/diff/ Testing (updated) --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Review Request 35477: DRILL-3268: queries with empty OVER() clause return empty result set

2015-06-15 Thread abdelhakim deneche
/diff/ Testing --- all unit tests are passing... Thanks, abdelhakim deneche

Review Request 35393: DRILL-3220: IOB Exception when using constants in window functions

2015-06-12 Thread abdelhakim deneche
.sql PRE-CREATION exec/java-exec/src/test/resources/window/q3218.sql PRE-CREATION exec/java-exec/src/test/resources/window/q3220.sql PRE-CREATION Diff: https://reviews.apache.org/r/35393/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 35393: DRILL-3220: IOB Exception when using constants in window functions

2015-06-12 Thread abdelhakim deneche
with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-08 Thread abdelhakim deneche
/oneKeyCountMultiBatch.json 09a405c exec/java-exec/src/test/resources/window/twoKeys.json f3ef4a5 exec/java-exec/src/test/resources/window/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing --- unit tests passing. customer/tpch100 passing Thanks, abdelhakim deneche

Re: Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-04 Thread abdelhakim deneche
exec/java-exec/src/test/resources/window/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-04 Thread abdelhakim deneche
/resources/window/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing (updated) --- unit tests passing. customer/tpch100 ongoing Thanks, abdelhakim deneche

Re: Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-04 Thread abdelhakim deneche
/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing (updated) --- unit tests passing. customer/tpch100 passing Thanks, abdelhakim deneche

Re: Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-03 Thread abdelhakim deneche
/java-exec/src/test/resources/window/twoKeys.json f3ef4a5 exec/java-exec/src/test/resources/window/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing --- Thanks, abdelhakim deneche

Review Request 34977: DRILL-3200: Add Window functions: ROW_NUMBER, RANK, PERCENT_RANK, DENSE_RANK and CUME_DIST

2015-06-02 Thread abdelhakim deneche
/test/resources/window/oneKeyCountMultiBatch.json 09a405c exec/java-exec/src/test/resources/window/twoKeys.json f3ef4a5 exec/java-exec/src/test/resources/window/twoKeysData.json fd09236 Diff: https://reviews.apache.org/r/34977/diff/ Testing --- Thanks, abdelhakim deneche

Re: Review Request 34829: DRILL-3190: Invalid FragmentState transition from CANCELLATION_REQUESTED in QueryManager

2015-06-01 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34829/#review86032 --- Ship it! Ship It! - abdelhakim deneche On June 1, 2015, 5:54

Re: Review Request 34829: DRILL-3190: Invalid FragmentState transition from CANCELLATION_REQUESTED in QueryManager

2015-05-29 Thread abdelhakim deneche
have one less class to check involved - abdelhakim deneche On May 29, 2015, 8:44 p.m., Sudheesh Katkam wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34829

Re: Review Request 34374: DRILL-3133: MergingRecordBatch can leak memory if query is canceled before batches in rawBatches were loaded

2015-05-28 Thread abdelhakim deneche
/ Testing (updated) --- all unit tests are passing along with functional and tpch100 Thanks, abdelhakim deneche

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-27 Thread abdelhakim deneche
://reviews.apache.org/r/34541/diff/ Testing --- still need to run the tests! Thanks, abdelhakim deneche

Re: Review Request 34690: DRILL-2903 Update TestDrillbitResilience tests so that they correctly manage canceled queries that get to complete too quickly

2015-05-27 Thread abdelhakim deneche
-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java https://reviews.apache.org/r/34690/#comment137077 ZookeeperHelper javadoc already explains what happens when we pass true to it's constructor. We don't need to repeat the explanation here. - abdelhakim deneche On May

Re: Review Request 34690: DRILL-2903 Update TestDrillbitResilience tests so that they correctly manage canceled queries that get to complete too quickly

2015-05-27 Thread abdelhakim deneche
/ --- (Updated May 27, 2015, 12:52 a.m.) Review request for drill, abdelhakim deneche, Chris Westin, and Venki Korukanti. Bugs: DRILL-2903 https://issues.apache.org/jira/browse/DRILL-2903 Repository: drill-git Description --- DRILL-2903: General improvements

Re: Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-05-22 Thread abdelhakim deneche
/#review85005 --- On May 22, 2015, 5:42 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34603

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-22 Thread abdelhakim deneche
PRE-CREATION Diff: https://reviews.apache.org/r/34541/diff/ Testing --- still need to run the tests! Thanks, abdelhakim deneche

Review Request 34603: DRILL-3167: When a query fails, Foreman should wait for all fragments to finish cleaning up before sending a FAILED state to the client

2015-05-22 Thread abdelhakim deneche
protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryResult.java 474e330 protocol/src/main/protobuf/UserBitShared.proto 68c8612 Diff: https://reviews.apache.org/r/34603/diff/ Testing --- unit tests are passing Thanks, abdelhakim deneche

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-21 Thread abdelhakim deneche
21, 2015, 7:34 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34541/ --- (Updated May 21, 2015, 7:34

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-21 Thread abdelhakim deneche
/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION Diff: https://reviews.apache.org/r/34541/diff/ Testing (updated) --- still need to run the tests! Thanks, abdelhakim deneche

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-21 Thread abdelhakim deneche
/#review84727 --- On May 21, 2015, 7:34 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34541

Re: Review Request 34541: DRILL-3147: tpcds-sf1-parquet query 73 causes memory leak

2015-05-21 Thread abdelhakim deneche
/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java PRE-CREATION Diff: https://reviews.apache.org/r/34541/diff/ Testing --- unit tests and tpch100 are passing. Couldn't run functional yet Thanks, abdelhakim deneche

Re: Review Request 34191: DRILL-3052: Canceling a fragment executor before it starts running will cause the Foreman to wait indefinitely for a terminal message from that fragment

2015-05-14 Thread abdelhakim deneche
. This shouldn't hold off this patch though, as it's more likely a separate problem - abdelhakim deneche On May 14, 2015, 5:25 a.m., Sudheesh Katkam wrote: --- This is an automatically generated e-mail. To reply, visit: https

Re: Review Request 34184: DRILL-3065: Memory Leak at ExternalSortBatch

2015-05-14 Thread abdelhakim deneche
/TestDrillbitResilience.java https://reviews.apache.org/r/34184/#comment134939 Should this test be part of TestDrillbitResilience ? - abdelhakim deneche On May 14, 2015, 9:59 p.m., Sean Hsuan-Yi Chu wrote: --- This is an automatically

Re: Review Request 34245: DRILL-3063: TestQueriesOnLargeFile leaks memory with 16M limit

2015-05-14 Thread abdelhakim deneche
/FragmentExecutor.java https://reviews.apache.org/r/34245/#comment134955 if we return here, this fragment will never send it's final state to the Foreman - abdelhakim deneche On May 15, 2015, 12:10 a.m., Chris Westin wrote

Re: Review Request 34184: DRILL-3065: Memory Leak at ExternalSortBatch

2015-05-14 Thread abdelhakim deneche
/TestDrillbitResilience.java https://reviews.apache.org/r/34184/#comment134951 actually I got confused in my previous comment. Why this unit test is inside TestDrillbitResilience ? - abdelhakim deneche On May 14, 2015, 11:14 p.m., Sean Hsuan-Yi Chu wrote

Re: Review Request 34191: DRILL-3052: Canceling a fragment executor before it starts running will cause the Foreman to wait indefinitely for a terminal message from that fragment

2015-05-14 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34191/#review83870 --- +1 (non binding) - abdelhakim deneche On May 14, 2015, 11:17 p.m

Review Request 34253: DRILL-3061: Fix memory leaks in TestDrillbitResilience

2015-05-14 Thread abdelhakim deneche
/ Testing --- not tested yet!!! Thanks, abdelhakim deneche

Re: Review Request 34253: DRILL-3061: Fix memory leaks in TestDrillbitResilience

2015-05-14 Thread abdelhakim deneche
/drill/exec/server/TestDrillbitResilience.java f95fbe1 Diff: https://reviews.apache.org/r/34253/diff/ Testing --- not tested yet!!! Thanks, abdelhakim deneche

Re: Review Request 34255: DRILL-3072: Root fragment status update consists of too many tasks

2015-05-14 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34255/#review83889 --- +1 (non binding) - abdelhakim deneche On May 15, 2015, 2:46 a.m

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
: https://reviews.apache.org/r/34173/#review83655 --- On May 13, 2015, 5:54 p.m., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https

Review Request 34179: DRILL-3046 Part 1: Memory Leak after cancelling a query

2015-05-13 Thread abdelhakim deneche
, abdelhakim deneche

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
., abdelhakim deneche wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34173/ --- (Updated May 13, 2015, 9:07 p.m.) Review

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
/drill/TestAllocationException.java PRE-CREATION Diff: https://reviews.apache.org/r/34173/diff/ Testing --- pending results from unit tests / cluster Thanks, abdelhakim deneche

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
/TestAllocationException.java PRE-CREATION Diff: https://reviews.apache.org/r/34173/diff/ Testing (updated) --- unit tests are passing... Thanks, abdelhakim deneche

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java PRE-CREATION Diff: https://reviews.apache.org/r/34173/diff/ Testing (updated) --- testing... Thanks, abdelhakim deneche

Re: Review Request 34184: DRILL-3065: Memory Leak at ExternalSortBatch

2015-05-13 Thread abdelhakim deneche
? - abdelhakim deneche On May 14, 2015, 12:55 a.m., Sean Hsuan-Yi Chu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34184/ --- (Updated

Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
-CREATION Diff: https://reviews.apache.org/r/34173/diff/ Testing --- pending results from unit tests / cluster Thanks, abdelhakim deneche

Re: Review Request 34173: DRILL-3053: add unchecked exception injection site in ChildAllocator

2015-05-13 Thread abdelhakim deneche
/ExecutionControlsInjector.java 387d300 exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java PRE-CREATION Diff: https://reviews.apache.org/r/34173/diff/ Testing --- pending results from unit tests / cluster Thanks, abdelhakim deneche

Re: Review Request 34071: DRILL-3034: Apply UserException to port-binding; handle in embedded-Drill case.

2015-05-12 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34071/#review83446 --- +1 (non binding) - abdelhakim deneche On May 12, 2015, 5:49 p.m

Re: Review Request 34071: DRILL-3034: Apply UserException to port-binding; handle in embedded-Drill case.

2015-05-12 Thread abdelhakim deneche
this will alter the user exception's message. Sometimes this will clutter the original error message making it less useful to the user - abdelhakim deneche On May 12, 2015, 5:59 a.m., Daniel Barclay wrote: --- This is an automatically

Re: Review Request 34008: DRILL:2977: In WorkManager, startFragmentPendingRemote() and addFragmentRunner() need to be permuted

2015-05-11 Thread abdelhakim deneche
/WorkManager.java https://reviews.apache.org/r/34008/#comment134128 We should remove the fragment manager from the work bus if it's fragment executor has already been cancelled. Otherwise, it will stay in the work bus forever. - abdelhakim deneche On May 10, 2015, 7:13 p.m., Sudheesh Katkam wrote

Re: Review Request 33949: DRILL-2476 Handle IterOutcome.STOP in buildSchema()

2015-05-11 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33949/#review83284 --- +1 (non binding) - abdelhakim deneche On May 11, 2015, 8:36 p.m

Re: Review Request 34008: DRILL:2977: In WorkManager, startFragmentPendingRemote() and addFragmentRunner() need to be permuted

2015-05-11 Thread abdelhakim deneche
the work event bus, so you don't need to pass the WorkerBee - abdelhakim deneche On May 11, 2015, 11:16 p.m., Sudheesh Katkam wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34008

Re: Review Request 34008: DRILL:2977: In WorkManager, startFragmentPendingRemote() and addFragmentRunner() need to be permuted

2015-05-11 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34008/#review83330 --- Ship it! Ship It! - abdelhakim deneche On May 12, 2015, 12:32

Re: Review Request 34063: DRILL-2776: add extensive tests for empty population coverage

2015-05-11 Thread abdelhakim deneche
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34063/#review83314 --- LGTM, +1 (non binding) - abdelhakim deneche On May 11, 2015, 11

  1   2   >