[GitHub] drill pull request: DRILL-3987: Componentize Drill, extracting vec...

2015-11-11 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/250#issuecomment-155847895 I've sent you the location of the branch many times; the remaining changes are still there: https://github.com/cwestin/incubator-drill/tree/alloc . After DRILL-3940

[GitHub] drill pull request: DRILL-3987: Componentize Drill, extracting vec...

2015-11-11 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/250#issuecomment-155957315 And I responded to the feedback on JIRA-3940, wherein I disagree with your comments; are you saying your word is final there? After that one, there would be one

[GitHub] drill pull request: DRILL-3987: Componentize Drill, extracting vec...

2015-11-11 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/250#issuecomment-155948627 On 2.2 above: This description of events is not entirely accurate. We have never held up releases for the allocator work. Not once, never mind numerous occasions

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42029468 --- Diff: common/src/main/java/org/apache/drill/common/DeferredException.java --- @@ -47,6 +75,15 @@ public void addException(final Exception exception

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42029661 --- Diff: common/src/main/java/org/apache/drill/common/DeferredException.java --- @@ -123,6 +160,9 @@ public void suppressingClose(final AutoCloseable

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42050675 --- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java --- @@ -369,24 +370,33 @@ AvaticaFactory getFactory() { return

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42039460 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java --- @@ -369,6 +378,10 @@ public void queryCompleted(QueryState state

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42050587 --- Diff: exec/jdbc-all/pom.xml --- @@ -420,7 +420,6 @@ org/apache/drill/exec/server/rest/** org/apache/drill/exec/rpc

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42040416 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java --- @@ -45,6 +45,7 @@ public class ImplCreator

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/199#issuecomment-148167171 I don't understand your comment, Jacques. OutOfMemoryException exists, and is a lot more specific than RuntimeException. Otherwise, what is OutOfMemoryException

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42039233 --- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java --- @@ -278,9 +265,9 @@ public String toString() { return this.root

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42061711 --- Diff: exec/jdbc-all/pom.xml --- @@ -420,7 +420,6 @@ org/apache/drill/exec/server/rest/** org/apache/drill/exec/rpc

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-14 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/199#discussion_r42028942 --- Diff: common/src/main/java/org/apache/drill/common/DrillAutoCloseables.java --- @@ -0,0 +1,50 @@ +/** + * Licensed to the Apache Software

[GitHub] drill pull request: DRILL-3927: use OutOfMemoryException in more p...

2015-10-13 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/199 DRILL-3927: use OutOfMemoryException in more places Some code cleanup required for the upcoming introduction of the rewritten direct memory allocator. Chiefly the introduction

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41908196 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41908122 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41907849 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java --- @@ -355,17 +353,19 @@ public void generateTestData(int values

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41907790 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java --- @@ -355,17 +353,19 @@ public void generateTestData(int values

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41908307 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41908273 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-13 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/194#issuecomment-147814132 I made the changes indicated above, but not any that might destabilize tests (noted above). Please re-review and merge. --- If your project is set up for it, you can

[GitHub] drill pull request: DRILL-3930: Remove direct references to TopLev...

2015-10-13 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/200 DRILL-3930: Remove direct references to TopLevelAllocator from unit t… …ests Ensure RootAllocatorFactory is used throughout the code so that we can change allocators via

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-12 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41811916 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-12 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41811855 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-12 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41812350 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -223,14 +231,187 @@ public void testNullableVarLen2

[GitHub] drill pull request: DRILL-3920 Add vector loading tests

2015-10-12 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/194#discussion_r41812282 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java --- @@ -174,9 +179,9 @@ public void

[GitHub] drill pull request: DRILL-1942-vector-test

2015-10-11 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/194#issuecomment-147255263 This is now https://issues.apache.org/jira/browse/DRILL-3920 . --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] drill pull request: DRILL-1942-vector-test

2015-10-09 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/194 DRILL-1942-vector-test Additional tests added to TestValueVectors for serialization and loading; these tests were used to find and debug issues with DrillBuf slicing. Some light cleanup of a few

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-05 Thread cwestin
Github user cwestin closed the pull request at: https://github.com/apache/drill/pull/181 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-05 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/181#issuecomment-145645022 This was merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-02 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/181#issuecomment-145033096 I tried to make the getBufferSize()-calls-getBufferSizeFor() change that you suggested, Jason, but it causes a lot of unit test failures, so there must be something else

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-02 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/181#issuecomment-145149090 I just squashed and pushed with the valueCount + 1 for the BaseRepeatedValueVector, along with using valueCount in getBufferSizeFor() instead of getAccessor

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-01 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/181 DRILL-3874: flattening large JSON objects uses too much direct memory - add getBufferSizeFor() to ValueVector interface - add implememtations of getBufferSizeFor() for all ValueVector derivatives

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-01 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/181#discussion_r40985372 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/BaseRepeatedValueVector.java --- @@ -123,6 +123,15 @@ public int getBufferSize

[GitHub] drill pull request: DRILL-3874: flattening large JSON objects uses...

2015-10-01 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/181#issuecomment-144898125 Parth: Re ObjectVector: I don't know what that's for. I just followed the pattern: getBufferSize() already throws that exception. Re OUTPUT_MEMORY_LIMIT: what do

[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...

2015-09-22 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/163#discussion_r40109454 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/memory/AtomicRemainder.java --- @@ -76,16 +74,17 @@ public void setLimit(long limit

[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...

2015-09-21 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/163#discussion_r40023098 --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/memory/TestAllocators.java --- @@ -63,6 +63,29 @@ private final static String planFile

[GitHub] drill pull request: DRILL-3811: AtomicRemainder incorrectly accoun...

2015-09-21 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/163#issuecomment-142102955 Just one comment, otherwise +1 (non-binding). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] drill pull request: DRILL-1942-readers:

2015-09-11 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/154 DRILL-1942-readers: - add extends AutoCloseable to RecordReader, and rename cleanup() to close() on derived classes. - fix many warnings - formatting fixes unit tests pass

[GitHub] drill pull request: DRILL-1942-hygiene:

2015-09-11 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/133#discussion_r39320204 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java --- @@ -51,11 +48,12 @@ public BufferManager(BufferAllocator allocator

[GitHub] drill pull request: DRILL-1942-hygiene

2015-09-10 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/120#discussion_r39197169 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java --- @@ -211,8 +220,11 @@ public TransferPair makeTransferPair(ValueVector

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-09-10 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/105#issuecomment-139409971 Addressed Sudheesh's comments. Unless Jason has anything else, can we please get this merged now? --- If your project is set up for it, you can reply to this email

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-09-10 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/105#issuecomment-139377475 I added code to collect errors, as well as to interrupt the test thread when an error occurs. --- If your project is set up for it, you can reply to this email and have

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-09-10 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r39216415 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,177 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-1942-hygiene

2015-09-10 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/120#discussion_r39196959 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java --- @@ -38,36 +37,43 @@ * @return True

[GitHub] drill pull request: DRILL-3598: use a factory to create the root a...

2015-09-09 Thread cwestin
Github user cwestin closed the pull request at: https://github.com/apache/drill/pull/104 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-09-09 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r39109156 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,199 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-3598: use a factory to create the root a...

2015-09-09 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/104#issuecomment-139064541 It looks like you've merged this -- is there any reason not to close the pull request? --- If your project is set up for it, you can reply to this email and have your

[GitHub] drill pull request: Alloc nolocks

2015-08-25 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/132 Alloc nolocks DRILL-1942-hygiene (another preparatory patch): - add AutoCloseable to some classes - minor fixes - formatting Unit tests pass Regression suite

[GitHub] drill pull request: DRILL-1942-hygiene:

2015-08-25 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/133 DRILL-1942-hygiene: DRILL-1942-hygiene (another preparatory patch): - add AutoCloseable to some classes - minor fixes - formatting Unit tests pass Regression

[GitHub] drill pull request: Alloc nolocks

2015-08-25 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/132#issuecomment-134781617 Opened by mistake. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] drill pull request: DRILL-1942-hygiene

2015-08-18 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/120 DRILL-1942-hygiene - Formatting - @Overrides - finals - some AutoCloseable additions needed for upcoming patches - new isCancelled() abstract method on FragmentManager, implemented

[GitHub] drill pull request: DRILL-2625: StackTrace format to match JDK's T...

2015-08-12 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/109 DRILL-2625: StackTrace format to match JDK's Throwable's format Small changes to the StackTrace class to make its output match the JDK's Throwable format so that IDEs and other tools can parse

[GitHub] drill pull request: DRILL-1942-templates: template changes with a ...

2015-08-12 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/108 DRILL-1942-templates: template changes with a few related dependencies. Changes to template files in the allocator branch, along with a few related dependencies. Unit tests pass, and regression

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-08-11 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r36806255 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,177 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-08-11 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r36811120 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,177 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-08-11 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r36817993 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,177 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-08-11 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/105#discussion_r36817701 --- Diff: exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributedConcurrent.java --- @@ -0,0 +1,177 @@ +/** + * Licensed to the Apache

[GitHub] drill pull request: DRILL-1942-concurrency-test: new smoke test fo...

2015-08-06 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/105 DRILL-1942-concurrency-test: new smoke test for concurrent query exec… …ution; useful to checking on the new allocator's locking schemes. Made the SilentListener

[GitHub] drill pull request: DRILL-3598: use a factory to create the root a...

2015-08-04 Thread cwestin
GitHub user cwestin opened a pull request: https://github.com/apache/drill/pull/104 DRILL-3598: use a factory to create the root allocator. - made the constructor for TopLevelAllocator package private to enforce this - other file cleanup in files the factory was added

[GitHub] drill pull request: DRILL-2650: Mark query end time when closing t...

2015-07-10 Thread cwestin
Github user cwestin commented on the pull request: https://github.com/apache/drill/pull/80#issuecomment-120554910 Do we want to stick with .info() for these messages instead of .debug()? Asking because I'm not sure, but it seems like noise. Otherwise, non-binding ship

[GitHub] drill pull request: DRILL-2650: Mark query end time when closing t...

2015-07-10 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/80#discussion_r34406523 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java --- @@ -789,7 +794,7 @@ protected void processEvent(final StateEvent

[GitHub] drill pull request: DRILL-2650: Mark query end time when closing t...

2015-07-10 Thread cwestin
Github user cwestin commented on a diff in the pull request: https://github.com/apache/drill/pull/80#discussion_r34406507 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java --- @@ -695,7 +697,10 @@ public void close