[jira] [Resolved] (DRILL-5462) Drill needs a per-query option to force a sort for non-covering indexed queries

[Chris Westin (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-5462?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Westin resolved DRILL-5462.
-
Resolution: Invalid

> Drill needs a per-query option to force a sort for non-covering indexed 
> queries
> ---
>
> Key: DRILL-5462
> URL: https://issues.apache.org/jira/browse/DRILL-5462
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Execution - Relational Operators
>Affects Versions: 1.10.0
>    Reporter: Chris Westin
>
> For storage systems like MapR-DB, which don't have a consistent read/snapshot 
> model, it's possible for records to change between the time they are found in 
> an index, and the time they are looked up in a primary table. If the index is 
> being relied upon to satisfy a sort, then it's possible that by the time the 
> rows are fetched from the primary table that they are no longer in sorted 
> order.
> Sometimes developers depend on an "order by" returning rows in the correct 
> order. In order to make sure that happens, there needs to be a per-query 
> option to force a sort on the result set in this case.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Created] (DRILL-5462) Drill needs a per-query option to force a sort for non-covering indexed queries

[Chris Westin (JIRA)]

Chris Westin created DRILL-5462:
---

 Summary: Drill needs a per-query option to force a sort for 
non-covering indexed queries
 Key: DRILL-5462
 URL: https://issues.apache.org/jira/browse/DRILL-5462
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Relational Operators
Affects Versions: 1.10.0
Reporter: Chris Westin


For storage systems like MapR-DB, which don't have a consistent read/snapshot 
model, it's possible for records to change between the time they are found in 
an index, and the time they are looked up in a primary table. If the index is 
being relied upon to satisfy a sort, then it's possible that by the time the 
rows are fetched from the primary table that they are no longer in sorted order.

Sometimes developers depend on an "order by" returning rows in the correct 
order. In order to make sure that happens, there needs to be a per-query option 
to force a sort on the result set in this case.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

[jira] [Created] (DRILL-5189) There's no documentation for the typeof() function

[Chris Westin (JIRA)]

Chris Westin created DRILL-5189:
---

 Summary: There's no documentation for the typeof() function
 Key: DRILL-5189
 URL: https://issues.apache.org/jira/browse/DRILL-5189
 Project: Apache Drill
  Issue Type: Bug
  Components: Documentation
Reporter: Chris Westin


I looked through the documentation at https://drill.apache.org/docs/ under SQL 
Reference > SQL Functions > ... and could not find any reference to typeof(). 
Google searches only turned up a reference to DRILL-4204.




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Request: new JIRA component "Client - Java"

[Chris Westin]

I'm becoming a user of DrillClient as a means to sending queries to Drill.
There isn't a good component to categorize JIRA tickets for that. I'd like
to request that someone with admin rights on the Drill JIRA create a new
component for "Client - Java"

Thanks!

[jira] [Created] (DRILL-5041) Make SingleRowListener (and other utilities) published public classes

[Chris Westin (JIRA)]

Chris Westin created DRILL-5041:
---

 Summary: Make SingleRowListener (and other utilities) published 
public classes
 Key: DRILL-5041
 URL: https://issues.apache.org/jira/browse/DRILL-5041
 Project: Apache Drill
  Issue Type: Bug
  Components: Client - C++
Affects Versions: 1.8.0
 Environment: This is actually for the Java Client, but there's no such 
component.
Reporter: Chris Westin


I have an application that uses the DrillClient interface (Specifically, an 
implementation of OJAI), and it would have been convenient to use things like 
SingleRowListener in my implementation, but they are not available outside the 
Drill project. There are many such utilities that are used in unit tests that 
would be useful to external API users.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3940) Make RecordBatch AutoCloseable

[Chris Westin (JIRA)]

Chris Westin created DRILL-3940:
---

 Summary: Make RecordBatch AutoCloseable
 Key: DRILL-3940
 URL: https://issues.apache.org/jira/browse/DRILL-3940
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Affects Versions: 1.2.0
Reporter: Chris Westin
Assignee: Chris Westin


This made it easier to find RecordBatches that were not cleaned up (because the 
compiler complains about AutoCloseable resources that haven't been closed).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3928) OutOfMemoryException should not be derived from FragmentSetupException

[Chris Westin (JIRA)]

Chris Westin created DRILL-3928:
---

 Summary: OutOfMemoryException should not be derived from 
FragmentSetupException
 Key: DRILL-3928
 URL: https://issues.apache.org/jira/browse/DRILL-3928
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Affects Versions: 1.2.0
Reporter: Chris Westin


Discovered while working on DRILL-3927.

The client and server both use the same direct memory allocator code. But the 
allocator's OutOfMemoryException is derived from FragmentSetupException (which 
is derived from ForemanException).

Firstly, OOM situations don't only happen during setup.

Secondly, Fragment and Foreman classes shouldn't exist on the client side. 
(This is causing unnecessary dependencies on the jdbc-all jar on server-only 
code).

There's nothing special in those base classes that OutOfMemoryException depends 
on. This looks like it was just a cheap way to avoid extra catch clauses in 
Foreman and FragmentExecutor by catching the baser classes only.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3927) Use OutOfMemoryException in more places

[Chris Westin (JIRA)]

Chris Westin created DRILL-3927:
---

 Summary: Use OutOfMemoryException in more places
 Key: DRILL-3927
 URL: https://issues.apache.org/jira/browse/DRILL-3927
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Reporter: Chris Westin
Assignee: Chris Westin


The new allocator uses OutOfMemoryException in better ways; some additional 
exception handling sites need to catch and handle this exception in preparation 
for the new allocator.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3930) Remove direct references to TopLevelAllocator from unit tests

[Chris Westin (JIRA)]

Chris Westin created DRILL-3930:
---

 Summary: Remove direct references to TopLevelAllocator from unit 
tests
 Key: DRILL-3930
 URL: https://issues.apache.org/jira/browse/DRILL-3930
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Affects Versions: 1.2.0
Reporter: Chris Westin
Assignee: Chris Westin


The RootAllocatorFactory should be used throughout the code to allow us to 
change allocators via configuration or other software choices. Some unit tests 
still reference TopLevelAllocator directly. We also need to do a better job of 
handling exceptions that can be handled by close()ing an allocator that isn't 
in the proper state (remaining open child allocators, outstanding buffers, etc).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3920) Add vector loading tests

[Chris Westin (JIRA)]

Chris Westin created DRILL-3920:
---

 Summary: Add vector loading tests
 Key: DRILL-3920
 URL: https://issues.apache.org/jira/browse/DRILL-3920
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Data Types
Affects Versions: 1.2.0
Reporter: Chris Westin
Assignee: Chris Westin


Add some additional tests to TestValueVector to test serialization and 
reloading of vectors, as well as the underlying buffer slicing operations that 
are used for this.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

How do I add projection capabilities to an operator that doesn't have them?

[Chris Westin]

In particular, I'm trying to work on
https://issues.apache.org/jira/browse/DRILL-3876 in order to reduce the
amount of memory downstream operators require.

I started looking at Projector.java and ProjectorTemplate.java, but it
seems like copying that pattern wouldn't be enough -- how do I get the
configuration that is passed in their setup to remove the large unneeded
column from the flatten output?

Re: Timing for 1.2 release, ideas around things to include

[Chris Westin]

 > > > >>> > >
> > > > >>> >
> > > > >>> > I've moved all the non-critical, non-active jiras out of 1.2.
> > > > However,
> > > > >>> that
> > > > >>> > still leaves 63 issues open. Do you want to do a quick hangout
> to
> > > try
> > > > >>> to
> > > > >>> > figure what we really want to include versus bump?
> > > > >>> >
> > > > >>> > J
> > > > >>> >
> > > > >>> > [1]
> > > > >>> >
> > > > >>> >
> > > > >>>
> > > >
> > >
> >
> https://gist.github.com/jacques-n/be0d2f584171d832aedb#file-release_commands
> > > > >>> > [2]
> > > > >>> >
> > > > >>> >
> > > > >>>
> > > >
> > >
> >
> https://calcite.incubator.apache.org/docs/howto.html#making-a-release-for-calcite-committers
> > > > >>> >
> > > > >>> >
> > > > >>> >
> > > > >>> > >
> > > > >>> > >
> > > > >>> > >
> > > > >>> > > On Thu, Oct 1, 2015 at 8:17 AM, Abdel Hakim Deneche <
> > > > >>> > adene...@maprtech.com
> > > > >>> > > >
> > > > >>> > > wrote:
> > > > >>> > >
> > > > >>> > > > I can manage the release.
> > > > >>> > > >
> > > > >>> > > > On Wed, Sep 30, 2015 at 6:10 PM, Aman Sinha <
> > > asi...@maprtech.com
> > > > >
> > > > >>> > wrote:
> > > > >>> > > >
> > > > >>> > > > > +1 for starting the process of finalizing the 1.2 release
> > and
> > > > >>> 'bounce
> > > > >>> > > > > everything not critical (and not in-progress or
> reviewable)
> > > off
> > > > >>> of
> > > > >>> > this
> > > > >>> > > > > release'.
> > > > >>> > > > >
> > > > >>> > > > > Aman
> > > > >>> > > > >
> > > > >>> > > > > On Wed, Sep 30, 2015 at 5:36 PM, Jacques Nadeau <
> > > > >>> jacq...@dremio.com>
> > > > >>> > > > > wrote:
> > > > >>> > > > >
> > > > >>> > > > > > Hey All,
> > > > >>> > > > > >
> > > > >>> > > > > > It seems like we should finalize on the 1.2 release
> > pretty
> > > > >>> soon. I
> > > > >>> > > > > > currently see something like 174 bugs currently open.
> Of
> > > > >>> those, ~20
> > > > >>> > > are
> > > > >>> > > > > > reviewable and ~20 are in progress. I'm inclined to
> > bounce
> > > > >>> > everything
> > > > >>> > > > not
> > > > >>> > > > > > critical (and not in-progress or reviewable) off of
> this
> > > > >>> release.
> > > > >>> > Any
> > > > >>> > > > > > thoughts on when everyone would like to do a release?
> > Also,
> > > > >>> anyone
> > > > >>> > > want
> > > > >>> > > > > to
> > > > >>> > > > > > be release manager?
> > > > >>> > > > > >
> > > > >>> > > > > > Jacques
> > > > >>> > > > > >
> > > > >>> > > > > > --
> > > > >>> > > > > > Jacques Nadeau
> > > > >>> > > > > > CTO and Co-Founder, Dremio
> > > > >>> > > > > >
> > > > >>> > > > > > On Tue, Aug 18, 2015 at 7:58 PM, Parth Chandra <
> > > > >>> par...@apache.org>
> > > > >>> > > > > wrote:
> > > > >>> > > &

[jira] [Created] (DRILL-3889) ValueVectors' implementations of getBufferSize() should call getBufferSizeFor()

[Chris Westin (JIRA)]

Chris Westin created DRILL-3889:
---

 Summary: ValueVectors' implementations of getBufferSize() should 
call getBufferSizeFor()
 Key: DRILL-3889
 URL: https://issues.apache.org/jira/browse/DRILL-3889
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Data Types
Affects Versions: 1.2.0
Reporter: Chris Westin
Priority: Minor


If getBufferSize() implementations call getBufferSizeFor() with their 
valueCount, then it should reduce duplicated code.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

How is a drillbit's death detected?

[Chris Westin]

I'm looking for information on how the determination is made that leads to
the notification that drillbits have died. There's an event
DrillbitUnregistered that gets sent around when this happens. How is it
triggered?

[jira] [Created] (DRILL-3874) flattening large JSON objects consumes too much direct memory

[Chris Westin (JIRA)]

Chris Westin created DRILL-3874:
---

 Summary: flattening large JSON objects consumes too much direct 
memory
 Key: DRILL-3874
 URL: https://issues.apache.org/jira/browse/DRILL-3874
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Data Types
Affects Versions: 1.1.0
Reporter: Chris Westin
Assignee: Chris Westin


A JSON record has a field whose value is an array with 20,000 elements; the 
record's size is 4MB. A select is used to flatten this. The query profile 
reports that the peak memory utilization was 8GB, most of it used by the 
flatten. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3876) Operators should not require a subsequent project to strip columns that aren't required

[Chris Westin (JIRA)]

Chris Westin created DRILL-3876:
---

 Summary: Operators should not require a subsequent project to 
strip columns that aren't required
 Key: DRILL-3876
 URL: https://issues.apache.org/jira/browse/DRILL-3876
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Affects Versions: 1.2.0
Reporter: Chris Westin






--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Has the execution of HashJoin changed recently?

[Chris Westin]

I've been looking into DRILL-1162, and found that a query that used to run
within certain constraints (DRILL_MAX_DIRECT_MEMORY=32G) no longer does
even though it looks like there should be plenty of memory. I took the
query in that report, and removed the last ten (redundant) join elements,
and it now fails with 32G direct memory, even though it previously ran
(although it produced the wrong results). When I check the query profile,
it only consumed around ~9G -- so there should be plenty of space left
before it fails. I started looking at it in the debugger, and the
allocation failure occurs during an attempt to resize the output vector.
The allocator being used believes there's no memory left, even though it's
parent has more than enough to satisfy the request.

I've also found another ticket with a HashJoin that fails in a similar way
even though there is plenty of memory.

Hash the execution of HashJoin or its use of its result vector changed in
some way recently?

[jira] [Resolved] (DRILL-2198) Vector re-allocation fails for large records

[Chris Westin (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-2198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Westin resolved DRILL-2198.
-
   Resolution: Duplicate
Fix Version/s: (was: 1.4.0)

This was fixed by DRILL-2731. I checked the vector template files 
(FixedValueVectors, NullableValueVectors, RepeatedValueVectors, and 
VariableLengthVectors), and all of them have wrapped reAlloc() with a loop to 
make sure the resulting vector is large enough to hold the required index.

> Vector re-allocation fails for large records
> 
>
> Key: DRILL-2198
> URL: https://issues.apache.org/jira/browse/DRILL-2198
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Execution - Data Types
>Reporter: Hanifi Gunes
>Priority: Minor
>
> In many places we reallocate vectors without making sure that there is enough 
> space after reallocation. This causes problem for large records that won't 
> fit into buffer even after reallocating once. We should keep re-allocating 
> until there is enough space for writing the record. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Drill community hangout starting in about 20 minutes (10am PST)

[Chris Westin]

Join us here:
https://plus.google.com/hangouts/_/event/ci4rdiju8bv04a64efj5fedd0lc

Apache Drill Developer Hangout Tuesday September 22, 2015 - Minutes

[Chris Westin]

Attendees: Chris Westin, Edmon Bergoli, Sudheesh Katkam, Daniel Barclay,
Jinfeng Ni, Venki Korukanti, Julien Le Dem

(*) Edmon has outstanding requests on mailing lists
- 1. plug-in for reading excel files
- 2. plug-in to read EDI files

Venki offered some suggestions re using the format plug-in to write these
plug-ins
- Venki will answer some of these on the dev list

Compute resources coming
- Sudheesh: Drill doesn't currently have any special handling for
co-processors
- available machines have up to 256G RAM
- have done some Spark testing in the past, so the support folks know
something
  about how to set up JVMs for this

(*) Daniel wants to talk about how empty files should be handled.
Message posted to dev list, but not answered.
- a filter pushdown made it appear like a file was empty (because nothing
passed the filter), so the schema information (OK_NEW_SCHEMA) was never
returned.

>From previous discussions: we're not good at handling empty files in
general.

The record handling protocol needs better definition; Daniel has come up
with
docs with help with Steven, but he's waiting for review and approval
DRILL-2888 (current bug) and DRILL-3641 (primarily the doc changes).

(*) Re upcoming release: Jacques shading change; Zookeeper used a different
version of Jackson than the one in the shaded jar.
- If using the jdbc-all jar, can't connect to Zookeeper, get ClassNotFound
- ticket currently assigned to Jacques

(*) Shaded test that isn't working
- might be easier to do that if we used maven integration tests
  - would that make it easier to run form an IDE?
- is someone at Dremio working on integration tests?

(*) A few people are going to Strata NYC next week

(*) Edmon wants to know the status of Java 1.8 Support.
- Venki: issue with order of values stored in the HashMap
- Chris: here's the umbrella ticket for the known issues:
https://issues.apache.org/jira/browse/DRILL-1491
That currently has one open sub-task left:
https://issues.apache.org/jira/browse/DRILL-1489

Re: Is anyone having issues with the jdbc unit tests (ITTestShadedJar)?

[Chris Westin]

I upgraded to maven 3.3.3 (from 3.2.3). I checked effective pom, and it's
antrun 1.7. However, the unit tests still get wedged in the same place.

On Wed, Sep 16, 2015 at 8:05 AM, Jacques Nadeau <jacq...@dremio.com> wrote:

> You can run a remote debugging session. However, given what you're
> describing, it sounds very much like an issue with the build rather than an
> issue with the Drill code or test. (Although a better error message would
> be nice for the situation where the app.class.path is missing.) As such,
> you would most likely need to debug the maven code to find out why the
> directive described below is failing.
>
> It seems like, on your system, maven-antrun-plugin isn't working correctly.
> The use of it in this module is very simple: generate a complete classpath
> and then put that classpath in an environment variable. In the plugin
> definition we're assigning the app.class.path variable the value of the
> value that the antrun plugin generated for the value of
> maven.test.classpath. (Note that you can't use this directly as this is
> something that is created by the antrun plugin and isn't generally
> available as a property inside of maven.) The code is here:
>
> https://github.com/apache/drill/blob/master/exec/jdbc-all/pom.xml#L215
>
> My guess is that your maven has a slightly different version of ant plugin
> that isn't handling this correctly. If that is the case, we can fix this by
> specifying a managed version of the antrun plugin. I'm on Maven 3.3.3 so
> when I run 'mvn help:effective-pom' and find the antrun plugin I see:
>
> 
>   maven-antrun-plugin
>   1.7
> 
>
> Can you confirm what you see? If you add 1.7 directly
> after line 215, does that resolve the issue?
>
>
>
> --
> Jacques Nadeau
> CTO and Co-Founder, Dremio
>
> On Wed, Sep 16, 2015 at 6:17 AM, Chris Westin <chriswesti...@gmail.com>
> wrote:
>
> > I only tried to run it from the IDE after it failed in mvn install (in
> > order to figure out why it's failing). Great, so if it can't work in the
> > IDE, how do we figure out why it's failing?
> > On Sep 16, 2015 5:37 AM, "Jacques Nadeau" <jacq...@dremio.com> wrote:
> >
> > > Ah, you're focused on testing from within the IDE?
> > >
> > > The level complexity of the build process to get a test to correctly
> test
> > > the right thing means jumping through a bunch of hoops to clear the
> > > classpath and then use a special classloader. I can't imagine that you
> > > could get it to run correctly in an ide. For example, Eclipse is very
> > > sloppy about keeping classpaths perfect versus what is declared in the
> > pom
> > > file.
> > >
> > > The parameter you're looking for is generated by the ant plugin simply
> > > because that appears the way to get the value into an environment
> > variable
> > > so that the inner classloader can load the drillbit for the test.
> > >
> > > The test: loads a drillbit in one classloader using the alternative
> > > classpath provided by the app.class.path variable. This is taken from
> > what
> > > would have typically been the jvm level classpath. We then clear the
> jvm
> > > classpath to only include the test class, Junit and  hamcrest. After
> the
> > > drillbit is initialized and we've run one query,  we then add the jdbc
> > all
> > > jar to the system classloader and open a connection to the drillbit and
> > > execute a query. The test is designed specifically to confirm that the
> > > requisite classes are correctly included in jdbc-all and that it will
> run
> > > correctly. The test can't run without the shaded jar being generated
> and
> > I
> > > can't imagine any of the of the ides have good enough understanding of
> > the
> > > various maven plugins and options used that they would correctly work.
> > Even
> > > if you found some changes that made the test execute in and ide, I
> can't
> > > imagine that it would correctly manage all the classpath stuff.
> > > On Sep 15, 2015 9:37 PM, "Chris Westin" <chriswesti...@gmail.com>
> wrote:
> > >
> > > > Variability: for me so far 2 out of 2 times.
> > > >
> > > > No stack trace, but as above, when I try to reproduce it in an IDE
> > > > "This seems to be because the test is getting an
> ExceptionInInitializer
> > > in
> > > > DrillbitClassLoader because the app.class.path property isn't set
> (and
> > > then
> > > > the resul

[jira] [Resolved] (DRILL-2166) left join with complex type throw ClassTransformationException

[Chris Westin (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-2166?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Westin resolved DRILL-2166.
-
Resolution: Fixed

> left join with complex type throw ClassTransformationException
> --
>
> Key: DRILL-2166
> URL: https://issues.apache.org/jira/browse/DRILL-2166
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Execution - Relational Operators
>Affects Versions: 0.8.0
>Reporter: Chun Chang
>    Assignee: Chris Westin
> Fix For: 1.2.0
>
>
> #Thu Jan 29 18:00:57 EST 2015
> git.commit.id.abbrev=09f7fb2
> Dataset can be downloaded from 
> https://s3.amazonaws.com/apache-drill/files/complex.json.gz
> The following query caused the exception:
> {code}
> 0: jdbc:drill:schema=dfs.drillTestDirComplexJ> select a.id, a.soa, b.sfa[0], 
> b.soa[1] from `complex.json` a left outer join `complex.json` b on 
> a.sia[0]=b.sia[0] order by a.id limit 20;
> Query failed: RemoteRpcException: Failure while running fragment., Line 35, 
> Column 32: No applicable constructor/method found for actual parameters "int, 
> int, org.apache.drill.exec.vector.complex.MapVector"; candidates are: "public 
> void org.apache.drill.exec.vector.NullableTinyIntVector.copyFromSafe(int, 
> int, org.apache.drill.exec.vector.NullableTinyIntVector)", "public void 
> org.apache.drill.exec.vector.NullableTinyIntVector.copyFromSafe(int, int, 
> org.apache.drill.exec.vector.TinyIntVector)" [ 
> fbf47be8-b5fe-4d56-9488-15d45d4224e4 on qa-node117.qa.lab:31010 ]
> [ fbf47be8-b5fe-4d56-9488-15d45d4224e4 on qa-node117.qa.lab:31010 ]
> Error: exception while executing query: Failure while executing query. 
> (state=,code=0)
> {code}
> stack from drill bit.log
> {code}
> 2015-02-04 13:37:22,117 [2b2d6eee-105b-5544-9111-83a3a356285d:frag:2:6] WARN  
> o.a.d.e.w.fragment.FragmentExecutor - Error while initializing or executing 
> fragment
> org.apache.drill.common.exceptions.DrillRuntimeException: 
> org.apache.drill.exec.exception.SchemaChangeException: 
> org.apache.drill.exec.exception.ClassTransformationException: 
> java.util.concurrent.ExecutionException: 
> org.apache.drill.exec.exception.ClassTransformationException: Failure 
> generating transformation classes for value:
> package org.apache.drill.exec.test.generated;
> import org.apache.drill.exec.exception.SchemaChangeException;
> import org.apache.drill.exec.ops.FragmentContext;
> import org.apache.drill.exec.record.RecordBatch;
> import org.apache.drill.exec.record.VectorContainer;
> import org.apache.drill.exec.vector.NullableBigIntVector;
> import org.apache.drill.exec.vector.NullableFloat8Vector;
> import org.apache.drill.exec.vector.NullableTinyIntVector;
> import org.apache.drill.exec.vector.complex.MapVector;
> import org.apache.drill.exec.vector.complex.RepeatedMapVector;
> {code}
> from forman drill bit.log
> {code}
> 2015-02-04 13:37:22,189 [BitServer-5] ERROR 
> o.a.d.exec.rpc.RpcExceptionHandler - Exception in pipeline.  Closing channel 
> between local /10.10.100.117:31012 and remote /10.10.100.120:56250
> io.netty.handler.codec.DecoderException: java.lang.NullPointerException
>   at 
> io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:99)
>  [netty-codec-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
>  [netty-transport-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
>  [netty-transport-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
>  [netty-codec-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
>  [netty-transport-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
>  [netty-transport-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:161)
>  [netty-codec-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
>  [netty-transport-4.0.24.Final.jar:4.0.24.Final]
>   at 
> io.netty.chan

[jira] [Resolved] (DRILL-1976) Possible Memory Leak in drill jdbc client when dealing with wide columns (5000 chars long)

[Chris Westin (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-1976?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Westin resolved DRILL-1976.
-
Resolution: Fixed

> Possible Memory Leak in drill jdbc client when dealing with wide columns 
> (5000 chars long)
> --
>
> Key: DRILL-1976
> URL: https://issues.apache.org/jira/browse/DRILL-1976
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Execution - Flow
>Reporter: Rahul Challapalli
>    Assignee: Chris Westin
> Fix For: 1.2.0
>
> Attachments: wide-strings-mod.sh, wide-strings.sh
>
>
> git.commit.id.abbrev=b491cdb
> I am seeing an execution failure when I execute the same query multiple times 
> (<10). The data file contains 9 columns out of which 7 are wide strings 
> (4000-5000 chars long)
> {code}
> select ws.*, sub.str_var str_var1 from widestrings ws INNER JOIN (select 
> str_var, max(tinyint_var) max_ti from widestrings group by str_var) sub on 
> ws.tinyint_var = sub.max_ti
> {code}
> Below are my memory settings :
> {code}
> DRILL_MAX_DIRECT_MEMORY="32G"
> DRILL_MAX_HEAP="4G"
> {code}
> Error From the JDBC client
> {code}
> select ws.*, sub.str_var str_var1 from widestrings ws INNER JOIN (select 
> str_var, max(tinyint_var) max_ti from widestrings group by str_var) sub on 
> ws.tinyint_var = sub.max_ti
> Exception in pipeline.  Closing channel between local /10.10.100.190:38179 
> and remote qa-node191.qa.lab/10.10.100.191:31010
> io.netty.handler.codec.DecoderException: java.lang.OutOfMemoryError: Direct 
> buffer memory
>   at 
> io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:151)
>   at 
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
>   at 
> io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
>   at 
> io.netty.channel.ChannelInboundHandlerAdapter.channelRead(ChannelInboundHandlerAdapter.java:86)
>   at 
> io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:333)
>   at 
> io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:319)
>   at 
> io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:787)
>   at 
> io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:130)
>   at 
> io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:511)
>   at 
> io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:468)
>   at 
> io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:382)
>   at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:354)
>   at 
> io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:116)
>   at java.lang.Thread.run(Thread.java:745)
> Caused by: java.lang.OutOfMemoryError: Direct buffer memory
>   at java.nio.Bits.reserveMemory(Bits.java:658)
>   at java.nio.DirectByteBuffer.(DirectByteBuffer.java:123)
>   at java.nio.ByteBuffer.allocateDirect(ByteBuffer.java:306)
>   at 
> io.netty.buffer.PoolArena$DirectArena.newUnpooledChunk(PoolArena.java:443)
>   at io.netty.buffer.PoolArena.allocateHuge(PoolArena.java:187)
>   at io.netty.buffer.PoolArena.allocate(PoolArena.java:165)
>   at io.netty.buffer.PoolArena.reallocate(PoolArena.java:280)
>   at io.netty.buffer.PooledByteBuf.capacity(PooledByteBuf.java:110)
>   at 
> io.netty.buffer.AbstractByteBuf.ensureWritable(AbstractByteBuf.java:251)
>   at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:849)
>   at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:841)
>   at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:831)
>   at io.netty.buffer.WrappedByteBuf.writeBytes(WrappedByteBuf.java:600)
>   at 
> io.netty.buffer.UnsafeDirectLittleEndian.writeBytes(UnsafeDirectLittleEndian.java:25)
>   at 
> io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:144)
>   ... 13 more
> Channel closed between local /10.10.100.190:38179 and remote 
> qa-node191.qa.lab/10.10.100.191:31010
> Channel is closed, discarding remaining 255231 byte(s) in buffer.
> {code}
> The logs



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Is anyone having issues with the jdbc unit tests (ITTestShadedJar)?

[Chris Westin]

Variability: for me so far 2 out of 2 times.

No stack trace, but as above, when I try to reproduce it in an IDE
"This seems to be because the test is getting an ExceptionInInitializer in
DrillbitClassLoader because the app.class.path property isn't set (and then
the resulting String.length() on its value throws an NPE)."

I don't see app.class.path set anywhere in any pom.xml (so it's not getting
set when I copy the surefire arguments into the IDE's launch configuration
for the test, either).


On Tue, Sep 15, 2015 at 9:09 PM, Jacques Nadeau <jacq...@dremio.com> wrote:

> It was tested on a clean machine a number of times. Any thoughts on the
> variability? Can you provide stack trace?
> On Sep 15, 2015 6:28 PM, "Sudheesh Katkam" <skat...@maprtech.com> wrote:
>
> > Yes, I see this issue too.
> >
> > > On Sep 15, 2015, at 5:53 PM, Chris Westin <chriswesti...@gmail.com>
> > wrote:
> > >
> > > This seems to be because the test is getting an ExceptionInInitializer
> in
> > > DrillbitClassLoader because the app.class.path property isn't set (and
> > then
> > > the resulting String.length() on its value throws an NPE).
> > >
> > > Bueller?
> > >
> > > On Tue, Sep 15, 2015 at 5:20 PM, Chris Westin <chriswesti...@gmail.com
> >
> > > wrote:
> > >
> > >> I just rebased, and twice in a row I've gotten wedged running
> > >> org.apache.drill.jdbc.ITTestShadedJar
> > >>
> > >>
> > >>
> >
> >
>

Is anyone having issues with the jdbc unit tests (ITTestShadedJar)?

[Chris Westin]

I just rebased, and twice in a row I've gotten wedged running
org.apache.drill.jdbc.ITTestShadedJar

[jira] [Resolved] (DRILL-2050) Accountor closed with outstanding buffer

[Chris Westin (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-2050?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Westin resolved DRILL-2050.
-
Resolution: Fixed

Closing based on [~adeneche]'s verification described above.

> Accountor closed with outstanding buffer
> 
>
> Key: DRILL-2050
> URL: https://issues.apache.org/jira/browse/DRILL-2050
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Execution - RPC
>Affects Versions: 0.8.0
>Reporter: Chun Chang
>    Assignee: Chris Westin
> Fix For: 1.2.0
>
>
> git.branch=8d1e1affe86a5adca3bc17eeaf7520f0d379a393
> git.commit.time=20.01.2015 @ 23\:02\:03 PST
> The following tpcds-implala-sf1 automation query caused RemoteRpcException. 
> This query works most of the time. But fails on average once in every four or 
> five tries. Test data can be downloaded from
> http://apache-drill.s3.amazonaws.com/files/tpcds-sf1-parquet.tgz
> {code}
> 0: jdbc:drill:schema=dfs.drillTestDir> select
> . . . . . . . . . . . . . . . . . . .>   *
> . . . . . . . . . . . . . . . . . . .> from
> . . . . . . . . . . . . . . . . . . .>   (select
> . . . . . . . . . . . . . . . . . . .> i.i_manufact_id as imid,
> . . . . . . . . . . . . . . . . . . .> sum(ss.ss_sales_price) sum_sales
> . . . . . . . . . . . . . . . . . . .> -- avg(sum(ss.ss_sales_price)) 
> over (partition by i.i_manufact_id) avg_quarterly_sales
> . . . . . . . . . . . . . . . . . . .>   from
> . . . . . . . . . . . . . . . . . . .> item as i,
> . . . . . . . . . . . . . . . . . . .> store_sales as ss,
> . . . . . . . . . . . . . . . . . . .> date_dim as d,
> . . . . . . . . . . . . . . . . . . .> store as s
> . . . . . . . . . . . . . . . . . . .>   where
> . . . . . . . . . . . . . . . . . . .> ss.ss_item_sk = i.i_item_sk
> . . . . . . . . . . . . . . . . . . .> and ss.ss_sold_date_sk = 
> d.d_date_sk
> . . . . . . . . . . . . . . . . . . .> and ss.ss_store_sk = s.s_store_sk
> . . . . . . . . . . . . . . . . . . .> and d.d_month_seq in (1212, 1212 + 
> 1, 1212 + 2, 1212 + 3, 1212 + 4, 1212 + 5, 1212 + 6, 1212 + 7, 1212 + 8, 1212 
> + 9, 1212 + 10, 1212 + 11)
> . . . . . . . . . . . . . . . . . . .> and ((i.i_category in ('Books', 
> 'Children', 'Electronics')
> . . . . . . . . . . . . . . . . . . .>   and i.i_class in ('personal', 
> 'portable', 'reference', 'self-help')
> . . . . . . . . . . . . . . . . . . .>   and i.i_brand in 
> ('scholaramalgamalg #14', 'scholaramalgamalg #7', 'exportiunivamalg #9', 
> 'scholaramalgamalg #9'))
> . . . . . . . . . . . . . . . . . . .> or (i.i_category in ('Women', 
> 'Music', 'Men')
> . . . . . . . . . . . . . . . . . . .>   and i.i_class in ('accessories', 
> 'classical', 'fragrances', 'pants')
> . . . . . . . . . . . . . . . . . . .>   and i.i_brand in ('amalgimporto 
> #1', 'edu packscholar #1', 'exportiimporto #1', 'importoamalg #1')))
> . . . . . . . . . . . . . . . . . . .> and ss.ss_sold_date_sk between 
> 2451911 and 2452275 -- partition key filter
> . . . . . . . . . . . . . . . . . . .>   group by
> . . . . . . . . . . . . . . . . . . .> i.i_manufact_id,
> . . . . . . . . . . . . . . . . . . .> d.d_qoy
> . . . . . . . . . . . . . . . . . . .>   ) tmp1
> . . . . . . . . . . . . . . . . . . .> -- where
> . . . . . . . . . . . . . . . . . . .> --   case when avg_quarterly_sales > 0 
> then abs (sum_sales - avg_quarterly_sales) / avg_quarterly_sales else null 
> end > 0.1
> . . . . . . . . . . . . . . . . . . .> order by
> . . . . . . . . . . . . . . . . . . .>   -- avg_quarterly_sales,
> . . . . . . . . . . . . . . . . . . .>   sum_sales,
> . . . . . . . . . . . . . . . . . . .>   tmp1.imid
> . . . . . . . . . . . . . . . . . . .> limit 100;
> +++
> |imid| sum_sales  |
> +++
> Query failed: RemoteRpcException: Failure while running fragment., Attempted 
> to close accountor with 1 buffer(s) still allocatedfor QueryId: 
> 2b401913-4292-26d4-18b0-f105afe06121, MajorFragmentId: 2, MinorFragmentId: 0.
>   Total 1 allocation(s) of byte size(s): 771, at stack location:
>   
> org.apache.drill.exec.memory.TopLevelAllocator$ChildAllocator.takeOwnership(TopLevelAllocator.java:197)
>   
> org.apache.drill.exec.rpc.data.DataServer.handle(DataServer.java:119)
>   
> org.apache.drill.exec.rpc.data.DataServer.handle(DataServer.java:48)
>

Re: Maven build failing on checkstyle

[Chris Westin]

Since most editors and IDEs will strip trailing whitespace everywhere in a
file (code or comments), we should leave it in to avoid getting spurious
diffs.

On Thu, Sep 10, 2015 at 7:02 AM, Jacques Nadeau  wrote:

> Hey Edmon,
>
> I completely agree that Checkstyle can be a pain. I've worked on a couple
> projects where the rules are truly draconian. In Drill today, I think we
> have only the following rules:
>
> 1) No trailing whitespace
> 2) No If statements without brackets (other than ternary)
> 3) No imports of the wrong guava classes (there are two other versions that
> are on the classpath inside of other packages)
>
> So the check you hit actually has nothing to do with Javadocs. As far as I
> know, we have no requirements for Javadocs. I'm generally fine removing the
> trailing white space requirement specifically from Javadocs (but I'm not
> sure how easy that is in the Checkstyle plugin). I think that the trailing
> whitespace check does provide great use in code so I'd prefer to keep it.
>
> What do you think?
>
> --
> Jacques Nadeau
> CTO and Co-Founder, Dremio
>
> On Thu, Sep 10, 2015 at 4:39 AM, Edmon Begoli  wrote:
>
> > My proposal:
> >
> > 1) Loosen checks between 1.2 and 1.3 releases
> >
> > 2) let me and as many other people as they are willing to contribute
> effort
> > to add proper javadoc.
> > Ask any contributor touching methods to add few lines of missing javadoc
> >
> > 3) once done tighten up the build to the max enforcing all best styling
> > practices including javadoc on all public methods and classes.
> >
> > On Thursday, September 10, 2015, Edmon Begoli  wrote:
> >
> > > Long story short (sorry :-|) - does it make sense to have a build
> > > stopping check for \s+$ in a javadoc and not check and stop the build
> for
> > > missing or improper javadoc?
> > >
> > > On Thursday, September 10, 2015, Edmon Begoli  > > > wrote:
> > >
> > >> My observation on this, as a newcomer, is that it is a paradoxical
> for a
> > >> build to fail on extra space in a meaningful javadoc, but not on the
> > >> actual missing text, @param, @see or a @return sections on the public
> > >> methods.
> > >>
> > >> On many classes there is no javadoc at all. On some, it is invalid.
> > >>
> > >> This makes it very hard for a contributor to get started because some
> of
> > >> these classes are components of a  pattern in deep interface
> > implementation
> > >> and inheritance hierarchy where you need to know exactly what two or
> > three
> > >> methods are supposed to do in order to implement them.
> > >>
> > >> Examples:
> > >>
> > >> // New text reader, complies with the RFC 4180 standard for text/csv
> > files
> > >> public class CompliantTextRecordReader extends AbstractRecordReader {
> > >>
> > >> // checks to see if we are querying all columns(star) or individual
> > columns
> > >> @Override
> > >> public boolean isStarQuery() {
> > >>
> > >>
> > >> While I strongly believe in a self-commenting code, it would make it
> > >> million times easier if there was a some kind of enforcement either in
> > form
> > >> of a human code review on the pull request, or even a checkstyle that
> > >> requires, for example, a minimum of 20 words on a class/interface
> > javadoc
> > >> and a minimum of 10 on a public method. (Regex for the presence of
> > >> alphanumeric tokens or English words in a javadoc)
> > >>
> > >> Also, I *volonteer* to just write a javadoc for beginning, but I think
> > >> it needs to be there. Drill is just way too complex and way too
> > >> undocumented for an easy start for a contributor.
> > >>
> > >> Here is an example from POI for how to use the API. If you notice, a
> > >> javadoc is 5x the code because it is an intro stuff showing how to use
> > it:
> > >>
> > >>
> >
> http://svn.apache.org/repos/asf/poi/trunk/src/examples/src/org/apache/poi/ss/examples/ToCSV.java
> > >>
> > >> or, an example from Hive of some the core classes:
> > >>
> > >>
> >
> https://github.com/apache/hive/blob/master/storage-api/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java
> > >>
> > >> Thoughts?
> > >>
> > >>
> > >> On Thursday, September 10, 2015, Jacques Nadeau 
> > >> wrote:
> > >>
> > >>> Hey Ted,
> > >>>
> > >>> FYI, we added trailing spaces check on purpose.  Please open a
> > discussion
> > >>> rather than making a random decision. If anything our checkstyle is
> far
> > >>> too
> > >>> lenient which has led to poor consistency and missing comments.
> > >>> On Sep 9, 2015 11:18 AM, "Ted Dunning" 
> wrote:
> > >>>
> > >>> > Checkstyle is clearly being too picky here.
> > >>> >
> > >>> > The only problem with spaces at the end of a line is that some
> tools
> > >>> strip
> > >>> > them out automagically.  This leads to format changes that make
> > reviews
> > >>> > (very slightly) more difficult.
> > >>> >
> 

Re: zeroVectors() interface for value vectors

[Chris Westin]

Maybe we should start by putting these rules in a comment in the value
vector base interfaces? The lack of such information is why there are
deviations and other expectations.

On Tue, Aug 25, 2015 at 8:22 AM, Jacques Nadeau jacq...@dremio.com wrote:

 There are a few unspoken rules around vectors:

 - values need to be written in order (e.g. index 0, 1, 2, 5)
 - null vectors start with all values as null before writing anything
 - for variable width types, the offset vector should be all zeros before
 writing
 - you must call setValueCount before a vector can be read
 - you should never write to a vector once it has been read.

 The ultimate goal we should get to the point where you the interfaces
 guarantee this order of operation:

 allocate  mutate  setvaluecount  access  clear (or allocate to start
 the process over, xxx).  Any deviation from this pattern should result in
 exception.  We should do this only in debug mode as this code is extremely
 performance sensitive.  Operations like transfer should be built on top of
 this state model.  (In that case, it would mean src moves to clear state
 and target moves to access state.  It also means that transfer should only
 work in access state.)

 If we need special purpose data structures that don't operate in these
 ways, we should make sure to keep them separate rather than trying to
 accommodate a deviation from this pattern in the core vector code.

 I wrote xxx above because I see the purpose of zeroVectors as being a reset
 on the vector state back to the original state.  Maybe we should actually
 call it 'reset' rather than 'zeroVectors'.  This would basically pick up at
 mutate mode again.

 Since these rules were never formalized, I'm sure there are a few places
 where we currently deviate.  We should enforce these rules and then get
 those issues fixed.



 --
 Jacques Nadeau
 CTO and Co-Founder, Dremio

 On Tue, Aug 25, 2015 at 8:02 AM, Abdel Hakim Deneche 
 adene...@maprtech.com
 wrote:

  Another important point to keep in mind here: ValueVectorWriteExpression
  operates under the undocumented assumption that the destination vector
 is
  empty, this way it can safely skip writing null values. In the case of
  window functions I am using a value vector as an internal buffer to hold
  values between batches which voids the assumption.
  If this assumption is indeed correct then adding zeroVector to value
  vectors is indeed the way to go.
 
  On Mon, Aug 24, 2015 at 8:30 PM, Jacques Nadeau jacq...@dremio.com
  wrote:
 
   In general, let's try to avoid extending the core structures like value
   vector read and write expressions for a single operator. Zerovector is
   trivial to implement so let's resolve that way (trivial since the
   underlying vector already has it and we just need to delegate down).
   On Aug 24, 2015 3:36 PM, Aman Sinha amansi...@apache.org wrote:
  
I am reviewing Hakim's patch for DRILL-3668 (first_value window
  function
incorrect result).  His code uses ValueVectorWriteExpression to set
   values
in an internal batch which get re-used across different partitions of
  the
window function.  Ideally, we just want to zero out the vector rather
   than
calling clear() since clear() will release the buffer.
   
However, currently zeroVectors() is only supported by
 FixedWidthVector,
   not
VariableWidthVector.  * Should there be such an interface for
 variable
width ? * The implementation could zero out just the offset vector.
   
In the absence of such an interface, Hakim has added a boolean flag
witeNulls to ValueVectorWriteExpression (see
   
   
  
 
 https://github.com/adeneche/incubator-drill/commit/cab73cd1a50163dd25fe0f9c55c264780ea3616d
)
 and is conditionally doing the null-ing out in the generated code.
 It
won't affect the normal code path, it would get used for specific
  window
functions.
   
I am thinking of committing his patch and tracking the zeroVectors()
enhancement separately (if people agree that it would be useful).
 Let
  me
know...
   
Aman
   
  
 
 
 
  --
 
  Abdelhakim Deneche
 
  Software Engineer
 
http://www.mapr.com/
 
 
  Now Available - Free Hadoop On-Demand Training
  
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available
  
 

github pull requests: travis ci always fails

[Chris Westin]

I always see a message like

*continuous-integration/travis-ci/pr *— The Travis CI build could not
complete due to an error

even though patches are fine. Does anyone know what's up with that?

Re: Timing for 1.2 release, ideas around things to include

[Chris Westin]

Unfortunately, we've had to modify our plans, and are now thinking of
aiming for a vote on September 10th.

On Thu, Aug 13, 2015 at 1:29 PM, Chris Westin chriswesti...@gmail.com
wrote:

 We were thinking of targetting for a vote on August 26th.

 On Thu, Aug 13, 2015 at 1:19 PM, Jacques Nadeau jacq...@dremio.com
 wrote:

 Following up since we haven't discussed the 1.2 release in a while.  How
 are people feeling about trying to get a candidate out in the next week or
 two?

 thanks,
 Jacques

 --
 Jacques Nadeau
 CTO and Co-Founder, Dremio



  -- Forwarded message --
  From: Parth Chandra pchan...@maprtech.com
  Date: Thu, Jul 9, 2015 at 11:29 AM
  Subject: Re: Timing for 1.2 release, ideas around things to include
  To: dev@drill.apache.org
 
 
  I just moved DRILL-3201 to 1.2. It's one of the candidates for inclusion
  into 1.2
 
  On Thu, Jul 9, 2015 at 7:04 AM, michael.engl...@nomura.com wrote:
 
   I'm not sure what the appetite is for pushing this into 1.2 vs the
  planned
   1.3, but Drill UI Authentication would be really great for
  companies/users
   that run multi-tenant Hadoop clusters with Drill. At the moment any
 user
   can change storage plugins and cancel running queries, even if they
 did
  not
   submit them. This is a bit of a blocker for Drill on multi-tenant
   production clusters for now.
  
   JIRA link:
   https://issues.apache.org/jira/browse/DRILL-3201
  
  
  
   -Original Message-
   From: Jacques Nadeau [mailto:jacq...@apache.org]
   Sent: 08 July 2015 17:19
   To: dev@drill.apache.org
   Subject: Timing for 1.2 release, ideas around things to include
  
   Hey Guys,
  
   What do you think about planning the 1.2 feature cutoff around Aug
 21st
   and target a release vote a week or so after that?
  
   What are different features people want to get into 1.2?
  
   Here are couple of things that I'd like to help facilitate
 contribution
   and/or inclusion:
  
   - Mongo improvements (including extended type fixes 2879 , test cases
  1666,
   etc)
   - httpd log parser format plugin (3423)
   - rpc enhancements (local 2941 and off-thread 3242)
   - First merge of Magnus' JDBC connector (3180)
   - New allocator (1942)
  
   Other key goals?  What do people think about the proposed timing?
  
  
   This e-mail (including any attachments) is private and confidential,
 may
   contain proprietary or privileged information and is intended for the
  named
   recipient(s) only. Unintended recipients are strictly prohibited from
   taking action on the basis of information in this e-mail and must
 contact
   the sender immediately, delete this e-mail (and all attachments) and
   destroy any hard copies. Nomura will not accept responsibility or
  liability
   for the accuracy or completeness of, or the presence of any virus or
   disabling code in, this e-mail. If verification is sought please
 request
  a
   hard copy. Any reference to the terms of executed transactions should
 be
   treated as preliminary only and subject to formal written
 confirmation by
   Nomura. Nomura reserves the right to retain, monitor and intercept
 e-mail
   communications through its networks (subject to and in accordance with
   applicable laws). No confidentiality or privilege is waived or lost by
   Nomura by any mistransmission of this e-mail. Any reference to
 Nomura
  is
   a reference to any entity in the Nomura Holdings, Inc. group. Please
 read
   our Electronic Communications Legal Notice which forms part of this
  e-mail:
   http://www.Nomura.com/email_disclaimer.htm
  
  
 
 

Re: [DISCUSS] Insert into Table support

[Chris Westin]

Re #7 in the original post Select table syntax can specify constant values
for one or more columns:
I would have assumed the select list can have any expressions that can be
evaluated on a row from the source; that includes columns, expressions on
columns, or constants. It's probably not your intent, but the stated form
implies that all I get are column values and constants. Which is it?

On Mon, Jul 27, 2015 at 5:40 PM, Mehant Baid baid.meh...@gmail.com wrote:

 I wanted to start a conversation around supporting the Insert into Table
 feature. As of 1.2 we initially want to support inserting into a table with
 Parquet files. Support for Json, CSV and other sources will follow as
 future enhancements.

 Aman, Jinfeng, Neeraja and I had an initial discussion about this and
 Neeraja provided a good summary of our discussion (pasted below) also
 stating some of the requirements for this feature.

  A ) Support Insert into a non-partitioned table
 -

 Ex: INSERT INTO T1 [col1, col2, col3]  SELECT col4, col5, col6 from T2
 (Source table: T2, Target table T1)
 Requirements:

 1. Target table column list specification is optional for Insert statement
 2. When specified, the column list in the Insert statement should
contain all the columns present in the target table (i.e No support
for partial insert)
 3. The column names specified for the source table do not need to match
to the target table column names. Match is performed based on ordinal.
 4.   # of Source table columns specified must be same as # of target
table columns
 5. Types of specified source table columns must match to the types of
target table columns
 6. Specification of * is not allowed in the Select table syntax
 7. Select table syntax can specify constant values for one or more columns


  B ) Support insert into a partitioned table
 --

 Ex: INSERT INTO T1 col1, col2,col3  partition by col1,col2 SELECT
 col4,col,col6 from T2

  * Target column specification is required when inserting data into an
already partitioned table
  * Requirements A.3-A.7 above apply for insert into partitioned tables
as well
  * A partition by clause along with one or more columns is required
  * All the columns specified in partition by clause must exist in the
target column list
  * Partition by columns specified do not need to match to the list of
columns that the original table partitioned with (i.e if the
original table is partitioned with col1, col2,  new data during
insert can be partitioned by col3 or just with col1 or col2..)


 Couple of open questions from the design perspective are

 1. How do we perform validation. Validation of data types, number of
 columns being inserted etc. In addition to validation we need to make sure
 that when we insert into an existing tables we insert data with the
 existing column names (select column list can have different names). This
 poses problems around needing to know the metadata at planning time, two
 approaches that have been floating around are
 * DotDrill files: We can store metadata, partitioning columns and
 other useful information here and we can perform validation during planning
 time. However the challenges with introducing DotDrill files include
  - consistency between metadata and the actual data (Nothing
 preventing users to copy files directly).
  - security around DotDrill files (can be dealt in the same
 way we perform security checks for drill tables in hdfs)
  - interface to change the DotDrill file, in the case we need
 to add a column to the table or add a new partition etc.

 * Explicit Syntax/ No metadata approach: Another approach is to
 avoid DotDrill files and use explicit syntax to glean as much information
 as possible from the SQL statement itself. Some of the challenges with this
 approach are
  - Gathering metadata information: Since we have no idea what
 the existing schema is we would need to perform a mini scan to learn the
 schema at planning time to be able to perform some validation. The problem
 with this approach is how do we determine how many files we need to read in
 order to learn the schema? If we use a sample set and not all the files
 have the same schema,
 we could have non-deterministic results based on the
 sample of files read. Also reading all the files and merging the schema
 seems like an expensive cost to pay.
  - From the user's perspective, while inserting into a
 partitioned table, user will have to specify the partitioning columns again
 in the Insert statement, despite having specified the partition columns in
 the CTAS.

 2. What is a reasonable assumption for a Drill table in terms of changing
 schema. Having the same exact schema for all files in a table is too rigid
 an assumption at this point?

 One thing to remember with DotDrill file is to also the repercussions on
 Drop table, 

Re: anyone seen these errors on master ?

[Chris Westin]

Given that the difference is just

 java.lang.Exception: Unexpected exception,
 expectedorg.apache.drill.exec.exception.OversizedAllocationException but
 wasorg.apache.drill.exec.memory.OutOfMemoryRuntimeException

The question of what constitutes an oversized allocation? comes to mind.
Is this test fragile relative to being run in different environments?
I haven't seen the test so how is the determination that something is
oversized made? It seems like that criterion sometimes fails, and we get an
OOM because whatever the request is is still very large.


On Wed, Aug 5, 2015 at 2:26 PM, Hanifi Gunes hgu...@maprtech.com wrote:

 I don't seem to be able to re-prod this. Let me look at this and update you
 all.

 On Thu, Aug 6, 2015 at 12:03 AM, Abdel Hakim Deneche 
 adene...@maprtech.com
 wrote:

  I didn't make any change, I'm running 2 forks (the default). I got those
  errors 3 times now, 2 on a linux VM and 1 on a linux physical node
 
  On Wed, Aug 5, 2015 at 1:03 PM, Hanifi Gunes hgu...@maprtech.com
 wrote:
 
   Did you tighten your memory settings? How many forks are you running
  with?
   I bet you are truly running out of memory while executing this
 particular
   test case.
  
   -H+
  
   On Wed, Aug 5, 2015 at 8:56 PM, Sudheesh Katkam skat...@maprtech.com
   wrote:
  
b2bbd99 committed on July 6th introduced the test.
   
 On Aug 5, 2015, at 10:21 AM, Jinfeng Ni jinfengn...@gmail.com
  wrote:

 In that case,  we probably need do binary search to figure out
 which
recent
 patch is causing this problem.

 On Wed, Aug 5, 2015 at 10:03 AM, Abdel Hakim Deneche 
adene...@maprtech.com
 wrote:

 Just got those errors on master too

 On Wed, Aug 5, 2015 at 9:07 AM, Abdel Hakim Deneche 
adene...@maprtech.com

 wrote:

 I'm seeing those errors intermittently when building my private
branch, I
 don't believe I made any change that would have caused them.
 Anyone
seen
 them too ?



   
  
 
 testBitVectorReallocation(org.apache.drill.exec.record.vector.TestValueVector)
 Time elapsed: 2.043 sec   ERROR!
 java.lang.Exception: Unexpected exception,

   expectedorg.apache.drill.exec.exception.OversizedAllocationException
 but
 wasorg.apache.drill.exec.memory.OutOfMemoryRuntimeException
 at java.nio.Bits.reserveMemory(Bits.java:658)
 at java.nio.DirectByteBuffer.init(DirectByteBuffer.java:123)
 at java.nio.ByteBuffer.allocateDirect(ByteBuffer.java:306)
 at


   
  
 
 io.netty.buffer.UnpooledUnsafeDirectByteBuf.allocateDirect(UnpooledUnsafeDirectByteBuf.java:108)
 at


   
  
 
 io.netty.buffer.UnpooledUnsafeDirectByteBuf.init(UnpooledUnsafeDirectByteBuf.java:69)
 at


   
  
 
 io.netty.buffer.UnpooledByteBufAllocator.newDirectBuffer(UnpooledByteBufAllocator.java:50)
 at


   
  
 
 io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:155)
 at


   
  
 
 io.netty.buffer.PooledByteBufAllocatorL.newDirectBuffer(PooledByteBufAllocatorL.java:130)
 at


   
  
 
 io.netty.buffer.PooledByteBufAllocatorL.directBuffer(PooledByteBufAllocatorL.java:171)
 at


   
  
 
 org.apache.drill.exec.memory.TopLevelAllocator.buffer(TopLevelAllocator.java:100)
 at


   
  
 
 org.apache.drill.exec.memory.TopLevelAllocator.buffer(TopLevelAllocator.java:116)
 at
   org.apache.drill.exec.vector.BitVector.reAlloc(BitVector.java:139)
 at


   
  
 
 org.apache.drill.exec.record.vector.TestValueVector.testBitVectorReallocation(TestValueVector.java:125)




   
  
 
 testFixedVectorReallocation(org.apache.drill.exec.record.vector.TestValueVector)
 Time elapsed: 0.436 sec   ERROR!
 java.lang.Exception: Unexpected exception,

   expectedorg.apache.drill.exec.exception.OversizedAllocationException
 but
 wasorg.apache.drill.exec.memory.OutOfMemoryRuntimeException
 at java.nio.Bits.reserveMemory(Bits.java:658)
 at java.nio.DirectByteBuffer.init(DirectByteBuffer.java:123)
 at java.nio.ByteBuffer.allocateDirect(ByteBuffer.java:306)
 at


   
  
 
 io.netty.buffer.UnpooledUnsafeDirectByteBuf.allocateDirect(UnpooledUnsafeDirectByteBuf.java:108)
 at


   
  
 
 io.netty.buffer.UnpooledUnsafeDirectByteBuf.init(UnpooledUnsafeDirectByteBuf.java:69)
 at


   
  
 
 io.netty.buffer.UnpooledByteBufAllocator.newDirectBuffer(UnpooledByteBufAllocator.java:50)
 at


   
  
 
 io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:155)
 at


   
  
 
 io.netty.buffer.PooledByteBufAllocatorL.newDirectBuffer(PooledByteBufAllocatorL.java:130)
 at


   
  
 
 io.netty.buffer.PooledByteBufAllocatorL.directBuffer(PooledByteBufAllocatorL.java:171)
 at


   
  
 
 

[jira] [Created] (DRILL-3598) Use a factory to create the root allocator

[Chris Westin (JIRA)]

Chris Westin created DRILL-3598:
---

 Summary: Use a factory to create the root allocator
 Key: DRILL-3598
 URL: https://issues.apache.org/jira/browse/DRILL-3598
 Project: Apache Drill
  Issue Type: Improvement
  Components: Execution - Flow
Affects Versions: 1.1.0
Reporter: Chris Westin
Assignee: Chris Westin


Use a factory instead of the constructor for the top-level direct memory 
allocator so that we can replace which allocator gets instantiated.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Request - hold off on merging to master for 48 hours

[Chris Westin]

I've got a large patch that includes a completely rewritten direct memory
allocator (replaces TopLevelAllocator).

The space accounting is much tighter than with the current implementation,
and it catches a lot more problems than the current implementation does. It
also fixes issues with accounting around the use of shared buffers, and
buffer ownership transfer (used by the RPC layer to hand off buffers to
fragments that will do work).

It's been an ongoing battle to get this in, because every time I get close,
I rebase, and it finds more new problems (apparently introduced by other
work done since my last rebase). These take time to track down and fix,
because they're often in areas of the code I don't know.

It looks like I'm very close right now. I rebased against apache/master on
Friday. All the unit tests passed. All of our internal tests passed except
for one query, which takes an IllegalReferenceCountException (it looks like
a DrillBuf is being released one more time than it should be).

So, in order to keep the gap from getting wide again (it looks like I'm
already a couple of commits behind, but hopefully they don't introduce more
issues), I'm asking that folks hold off on merging into master for 48 hours
from now -- that's until about noon on Friday PST. I'm hoping that will
give me the time needed to finally get this in. If things go wrong with my
current patching, or I discover other problems, or can't find the illegal
reference count issue by then, I'll post a message and open things up
again. Meanwhile, you can still pull, do work, make pull requests, and get
them reviewed; just don't merge them to master.

Can we agree to this?

Chris

Re: Request - hold off on merging to master for 48 hours

[Chris Westin]

Ordinarily, I would agree. However, in this particular case, some other
folks wanted me closer to master so they could use my branch to track down
problems in new code. Also, the problems I was seeing were in code I'm not
familiar with, but there had been several recent commits claiming to fix
memory issues there. So I wanted to see if the problems I was seeing had
been taken care of. Sure enough, my initial testing shows that the problems
I was trying to fix had already been fixed by others -- they went away
after I rebased. In this case, chasing master saved me from having to track
all of those down myself, and duplicating the work. I'm hoping that there
weren't any significant new ones introduced. Testing is proceeding.

On Wed, Jul 29, 2015 at 1:59 PM, Parth Chandra par...@apache.org wrote:

 I think the idea (some of the idea is mine I'm afraid) is to allow Chris to
 catch up and rebase, not to have it reviewed and merged in two days.
 At the moment the problem is that every time he rebases, some test breaks
 and while he's chasing that down, the master moves ahead.
 If we get this 2 day break, we can get close enough to master and share the
 changes (a pre-review perhaps).
 Also, agree that perhaps the patch could be split into smaller pieces. Not
 renaming the allocator would in fact save several files from being included
 in the patch.


 P


 On Wed, Jul 29, 2015 at 12:17 PM, Jacques Nadeau jacq...@dremio.com
 wrote:

  In general, this type of request makes a lot of sense.  That said, we
  should get to +1 before we hold master.
 
  The last changes that were posted on DRILL-1942 are more than a month
 old.
  The patch touched ~100 files and was thousands of lines.  It had an issue
  that we identified.  Since then, you exposed very little to the community
  on your progress.  It seems unrealistic to provide a large update to this
  patch and expect review and merge within 48 hours.  Had you been
 exposing 
  discussing your work over the last month and the community agreed that it
  was ready for merge, your ask here would seem more feasible.
 
  My suggestion is to stop chasing master AND break your patch into smaller
  pieces.  Looking at the old patch, the vast majority of changes have very
  little to do with a new allocator functionality.  For example, renaming
 the
  allocator could be merged without changing the underlying implementation
  (that would substantially reduce the patch size).
 
  For the allocator specifically, let's get the code right first.  Then we
  can work as a community to minimize the effort to get it merged.
 
 
 
  --
  Jacques Nadeau
  CTO and Co-Founder, Dremio
 
  On Wed, Jul 29, 2015 at 11:41 AM, Chris Westin chriswesti...@gmail.com
  wrote:
 
   I've got a large patch that includes a completely rewritten direct
 memory
   allocator (replaces TopLevelAllocator).
  
   The space accounting is much tighter than with the current
  implementation,
   and it catches a lot more problems than the current implementation
 does.
  It
   also fixes issues with accounting around the use of shared buffers, and
   buffer ownership transfer (used by the RPC layer to hand off buffers to
   fragments that will do work).
  
   It's been an ongoing battle to get this in, because every time I get
  close,
   I rebase, and it finds more new problems (apparently introduced by
 other
   work done since my last rebase). These take time to track down and fix,
   because they're often in areas of the code I don't know.
  
   It looks like I'm very close right now. I rebased against apache/master
  on
   Friday. All the unit tests passed. All of our internal tests passed
  except
   for one query, which takes an IllegalReferenceCountException (it looks
  like
   a DrillBuf is being released one more time than it should be).
  
   So, in order to keep the gap from getting wide again (it looks like I'm
   already a couple of commits behind, but hopefully they don't introduce
  more
   issues), I'm asking that folks hold off on merging into master for 48
  hours
   from now -- that's until about noon on Friday PST. I'm hoping that will
   give me the time needed to finally get this in. If things go wrong with
  my
   current patching, or I discover other problems, or can't find the
 illegal
   reference count issue by then, I'll post a message and open things up
   again. Meanwhile, you can still pull, do work, make pull requests, and
  get
   them reviewed; just don't merge them to master.
  
   Can we agree to this?
  
   Chris
  
 

Re: dropping qualified names in logger declarations

[Chris Westin]

I tried to follow up with Hakim's suggestion of consulting the checkstyle
rules I expect to use (I've suggested before that we should start with
Google's rules as a basis, and make a few tweaks), but unfortunately, on
that day last week, SourceForge was down (that's where the rules are
hosted). It's finally back, so here they are
http://checkstyle.sourceforge.net/google_style.html . I can't seem to find
any guidance for this particular issue.

On Wed, Jul 22, 2015 at 10:51 AM, Daniel Barclay dbarc...@maprtech.com
wrote:


 Chris Westin wrote:

 For the special case of the logger, I kind of like it this way, because I
 can turn it off just by commenting out a single line (to get rid of
 unreferenced variable warnings),or add it by pasting in or uncommenting a
 single line. In either case I don't have to worry about removing or adding
 the import line separately, which can be quite far away if there are a lot
 of imports.


 Why not use the modern Java feature intended for cases like this:  have
 a @SuppressWarnings(unused) annotation on the logger member
 declaration if the declaration has been added but the member isn't used
 yet?

 Then:
 - We can still avoid unused-variable warnings for logger members that
   have already been declared before there are any uses.
 - We no longer have to move up top to adjust an already-existing logger
   declaration when adding a logger use down in the code.
   (Yes, we should remove (or comment out) the annotation if the change
   isn't temporary, but we don't have to do that immediately just to
   continue compiling.)
 - We can now use code completion when adding the first logger call to a
   previously unused logger, since the declaration is real (not just a
   comment).
 - We can still comment out/uncomment only a single line (the annotation)
   to switch between the no-logger-uses and some-logger-use cases.  (That
   is, if you don't want to have to re-add the suppression annotation if
   the last use of a logger is removed later, you can comment out, rather
   than delete, the annotation when the first logger use it added.
 - We no longer have to either go adjust imports or use qualified names
   to avoid having to adjust imports.
 - We stop having unnecessarily qualified names in the code.
 - and, finally ...:
 - We stop having those names' extra visual clutter and length, which
   makes it harder to notice when the class literal ends up wrong.
   (Note the mention of pasting above.)

 Daniel






 On Tue, Jul 21, 2015 at 6:12 PM, Daniel Barclay dbarc...@maprtech.com
 wrote:

  For logger member declarations, can we drop the current pattern of using
 qualified names (like this:

private static final org.slf4j.Logger logger =
 org.slf4j.LoggerFactory.getLogger(StoragePluginRegistry.class);

 ) and allow using imports and non-qualified names (as we do for almost
 everything else)?


 Using qualified names adds a lot of visual noise, and pushes the class
 literal farther to the right, making it easier to fail to notice that
 it doesn't match the containing class.

 Thanks,
 Daniel
 --
 Daniel Barclay
 MapR Technologies




 --
 Daniel Barclay
 MapR Technologies

Re: Suspicious direct memory consumption when running queries concurrently

[Chris Westin]

What I was trying to get at was that we might be creating more chunks
because the per-thread caches for all the different threads haven't yet
been filled. Hence my questions about the degree of control we have over
the thread pool Netty is using for RPC threads. But if the asymptotic
observation isn't bearing out, then that probably isn't the cause either.

On Mon, Jul 27, 2015 at 12:47 PM, Abdel Hakim Deneche adene...@maprtech.com
 wrote:

 @Jacques, my understanding is that chunks are not owned by specific a
 thread but they are part of a specific memory arena which is in turn only
 accessed by specific threads. Do you want me to find which threads are
 associated with the same arena where we have hanging chunks ?


 On Mon, Jul 27, 2015 at 11:04 AM, Jacques Nadeau jacq...@dremio.com
 wrote:

  It sounds like your statement is that we're cacheing too many unused
  chunks.  Hanifi and I previously discussed implementing a separate
 flushing
  mechanism to release unallocated chunks that are hanging around.  The
 main
  question is, why are so many chunks hanging around and what threads are
  they associated with.  A Jmap dump and analysis should allow you to do
  determine which thread owns the excess chunks.  My guess would be the RPC
  pool since those are long lasting (as opposed to the WorkManager pool,
  which is contracting).
 
  --
  Jacques Nadeau
  CTO and Co-Founder, Dremio
 
  On Mon, Jul 27, 2015 at 9:53 AM, Abdel Hakim Deneche 
  adene...@maprtech.com
  wrote:
 
   When running a set of, mostly window function, queries concurrently on
 a
   single drillbit with a 8GB max direct memory. We are seeing a
 continuous
   increase of direct memory allocation.
  
   We repeat the following steps multiple times:
   - we launch in iteration of tests that will run all queries in a
 random
   order, 10 queries at a time
   - after the iteration finishes, we wait for a couple of minute to give
   Drill time to release the memory being held by the finishing fragments
  
   Using Drill's memory logger (drill.allocator) we were able to get
   snapshots of how memory was internally used by Netty, we only focused
 on
   the number of allocated chunks, if we take this number and multiply it
 by
   16MB (netty's chunk size) we get approximately the same value reported
 by
   Drill's direct memory allocation.
   Here is a graph that shows the evolution of the number of allocated
  chunks
   on a 500 iterations run (I'm working on improving the plots) :
  
   http://bit.ly/1JL6Kp3
  
   In this specific case, after the first iteration Drill was allocating
  ~2GB
   of direct memory, this number kept rising after each iteration to ~6GB.
  We
   suspect this caused one of our previous runs to crash the JVM.
  
   If we only focus on the log lines between iterations (when Drill's
 memory
   usage is below 10MB) then all allocated chunks are at most 2% usage. At
   some point we end up with 288 nearly empty chunks, yet the next
 iteration
   will cause more chunks to be allocated!!!
  
   is this expected ?
  
   PS: I am running more tests and will update this thread with more
   informations.
  
   --
  
   Abdelhakim Deneche
  
   Software Engineer
  
 http://www.mapr.com/
  
  
   Now Available - Free Hadoop On-Demand Training
   
  
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available
   
  
 



 --

 Abdelhakim Deneche

 Software Engineer

   http://www.mapr.com/


 Now Available - Free Hadoop On-Demand Training
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available
 

Re: dropping qualified names in logger declarations

[Chris Westin]

For the special case of the logger, I kind of like it this way, because I
can turn it off just by commenting out a single line (to get rid of
unreferenced variable warnings), or add it by pasting in or uncommenting a
single line. In either case I don't have to worry about removing or adding
the import line separately, which can be quite far away if there are a lot
of imports.

On Tue, Jul 21, 2015 at 6:12 PM, Daniel Barclay dbarc...@maprtech.com
wrote:

 For logger member declarations, can we drop the current pattern of using
 qualified names (like this:

   private static final org.slf4j.Logger logger =
 org.slf4j.LoggerFactory.getLogger(StoragePluginRegistry.class);

 ) and allow using imports and non-qualified names (as we do for almost
 everything else)?


 Using qualified names adds a lot of visual noise, and pushes the class
 literal farther to the right, making it easier to fail to notice that
 it doesn't match the containing class.

 Thanks,
 Daniel
 --
 Daniel Barclay
 MapR Technologies

Re: question about UDF optimization

[Chris Westin]

Yep, I would expect pure to be the majority and default, and that makes
sense, because these functions are not class members that could have a
(implicit) this pointer that references member variables whose state
would change, leading to implementations with side-effects (and impure
results).

On Tue, Jul 21, 2015 at 5:25 PM, Ted Dunning ted.dunn...@gmail.com wrote:

 Even in my own warped experience, the vast majority of UDF's I have written
 or considered writing have been pure.



 On Tue, Jul 21, 2015 at 4:27 PM, Jacques Nadeau jacq...@dremio.com
 wrote:

  I don't think so.  There are something like 1500 functions where this
 isn't
  true (default) and one or two where it is.
 
  On Tue, Jul 21, 2015 at 4:25 PM, Daniel Barclay dbarc...@maprtech.com
  wrote:
 
  
   Should Drill be defaulting the other way?
  
   That is, instead of assuming pure unless declared otherwise (leading to
   wrong results in the case that the assumption is wrong (or the
 annotation
   was forgotten)), should Drill be assuming not pure unless declared pure
   (leading to only lower performance in the wrong-assumption case)?
  
   Daniel
  
  
  
  
   Jacques Nadeau wrote:
  
   There is an annotation on the function template.  I don't have a
 laptop
   close but I believe it is something similar to isRandom. It basically
   tells
   Drill that this is a nondeterministic function. I will be more
 specific
   once I get back to my machine if you don't find it sooner.
  
   Jacques
   *Summary:*
  
   Drill is very aggressive about optimizing away calls to functions with
   constant arguments. I worry that could extend to per record batch
   optimization if I accidentally have constant values and even if that
   doesn't happen, it is a pain in the ass now largely because Drill is
   clever
   enough to see through my attempt to hide the constant nature of my
   parameters.
  
   *Question:*
  
   Is there a way to mark a UDF as not being a pure function?
  
   *Details:*
  
   I have written a UDF to generate a random number.  It takes parameters
   that
   define the distribution.  All seems well and good.
  
   I find, however, that the function is only called once (twice,
 actually
   apparently due to pipeline warmup) and then Drill optimizes away later
   calls, apparently because the parameters to the function are constant
  and
   Drill thinks my function is a pure function.  If I make up some bogus
  data
   to pass in as a parameter, all is well and the function is called as
  much
   as I wanted.
  
   For instance, with the uniform distribution, my function takes two
   arguments, those being the minimum and maximum value to return.  Here
 is
   what I see with constants for the min and max:
  
   0: jdbc:drill:zk=local select random(0,10) from (values 5,5,5,5) as
   tbl(x);
   into eval
   into eval
   +-+
   |   EXPR$0|
   +-+
   | 1.7787372583008298  |
   | 1.7787372583008298  |
   | 1.7787372583008298  |
   | 1.7787372583008298  |
   +-+
  
  
   If I include an actual value, we see more interesting behavior even if
  the
   value is effectively constant:
  
   0: jdbc:drill:zk=local select random(0,x) from (values 5,5,5,5) as
   tbl(x);
   into eval
   into eval
   into eval
   into eval
   +--+
   |EXPR$0|
   +--+
   | 3.688377805419459|
   | 0.2827056410711032   |
   | 2.3107479622644918   |
   | 0.10813788169218574  |
   +--+
   4 rows selected (0.088 seconds)
  
  
   Even if I make the max value come along from the sub-query, I get the
  evil
   behavior although the function is now surprisingly actually called
 three
   times, apparently to do with warming up the pipeline:
  
   0: jdbc:drill:zk=local select random(0,max_value) from (select 14 as
   max_value,x from (values 5,5,5,5) as tbl(x)) foo;
   into eval
   into eval
   into eval
   +-+
   |   EXPR$0|
   +-+
   | 13.404462063773702  |
   | 13.404462063773702  |
   | 13.404462063773702  |
   | 13.404462063773702  |
   +-+
   4 rows selected (0.121 seconds)
  
   The UDF itself is boring and can be found at
   https://gist.github.com/tdunning/0c2cc2089e6cd8c030c0
  
   So how can I defeat this behavior?
  
  
  
   --
   Daniel Barclay
   MapR Technologies
  
 

Drill hangout starting now....

[Chris Westin]

https://plus.google.com/hangouts/_/event/ci4rdiju8bv04a64efj5fedd0lc

Re: Drill hangout starting now....

[Chris Westin]

Here are some brief notes I took during the hangout:

Attendees: Chris, Jacques, Byron, Parth, Sudheesh, Daniel, Hakim, Jim

Jacques: release schedule for the rest of the summer
proposes the 15th of each month for the next couple of months

Chris: I'd like to move to 6 weeks

It hasn't really been exactly 4 weeks yet anyway.

Propose a strategy around bug tagging in a more open way.
Until we have a strong commitment to do something, we should leave them as
Future.

Constantly pushing bugs into a future release seems like a poor use of
people's
time.

Parth: the problem with the future list, is it might just be a big dead
bucket.

Jacques asked about the state of the new buffer allocator...
Chris: I'm almost done with some changes that eliminate locking in favor
of transacted changes via a swap of an AtomicReference to an object that
holds the state-tracking variables; this is an optimistic locking scheme
that
doesn't span allocators; transfers between allocators cause local
transactions
within each allocator.

Jacques wants to get back to some old RPC tickets.

Is the recycler worth using for the allocator?
Jacques general rule: avoid objects for values within batches, but not for
record batches themselves.

Discussion of the use of the new serializing executor to deliver events to
fragment executors.

Hakim: we need to do some work with Parquet so we can catch up to them, and
so
they can take back our patches.

Parth: we're moving forward with Calcite in order to be current on that as
well.


On Tue, Jul 7, 2015 at 10:02 AM, Chris Westin chriswesti...@gmail.com
wrote:

 https://plus.google.com/hangouts/_/event/ci4rdiju8bv04a64efj5fedd0lc

Re: What is PStoreProvider?

[Chris Westin]

I believe it's for Persistent. I wouldn't mind if that were spelled out.
It gets used for storing things like query profiles. I believe there are a
couple of implementations, one for files, and one for Zookeeper.

On Wed, Jul 1, 2015 at 11:26 AM, Daniel Barclay dbarc...@maprtech.com
wrote:

 What does interface PStoreProvider represent?  (First of all, what does the
 P stand for?)

 (The interface declaration has no documentation comment.)

 Daniel

 --
 Daniel Barclay
 MapR Technologies

Improving the default JIRA assignments for those working on Drill full time

[Chris Westin]

It's become clear that the default assignments for JIRAs for some
components don't make a lot of sense. We'd like to clean that up.

If you work on Drill full-time, and are the default assignee for components
(you're the Automatic assignment in the assignee box), and it doesn't
make sense for you to be so, please speak up, and if possible, suggest an
alternate.

If you're not the default assignee for a component, and you think you
should be, please volunteer to take on that component.

Thanks,
Chris

Please maintain the Status of JIRAs you're working on

[Chris Westin]

If you're working on a JIRA, please make sure that you've marked it as IN
PROGRESS. Otherwise we may assign it to someone else or a future timeframe.

Please take a moment to check on this now.

Re: [DISCUSSION] should BufferAllocator.buffer() throw an OutOfMemoryException ?

[Chris Westin]

I would resist using a checked exception. We would end up having to add it
to almost everything everywhere. Or constantly wrapping it with
RuntimeException to get around that; in which case why make it checked to
begin with?

On Tue, Jun 30, 2015 at 1:28 PM, Hanifi GUNES h...@apache.org wrote:

 I would propose throwing a checked exception encouraging explicit and
 consistent handling of this event. Each sub-system has liberty to decide if
 an OOM failure is fatal or non-fatal depending on its capabilities. Also if
 at some point a sub-system needs to communicate with its callers via a
 different mechanism such like using flags (boolean, enum etc) or raising an
 unchecked exception that's still fine, just handle the exception. If there
 is a need to suppress the checked exception that's fine too, just use a
 helper method.

 Either way, returning *null* sounds problematic in many ways i) it is
 implicit ii) unsafe iii) its handling logic is repetitive iv) it is
 semantically unclean to make null mean something - even worse something
 context specific.


 -Hanifi

 2015-06-30 12:23 GMT-07:00 Abdel Hakim Deneche adene...@maprtech.com:

  I guess that would fix the issue too. But we may still run into
 situations
  where the caller will pass a flag to mute the exception and not handle
  the case anyway.
 
  If .buffer() unconditionally throws an exception, can't the caller, who
  wants to, just catch that and handle it properly ?
 
  On Tue, Jun 30, 2015 at 12:13 PM, Chris Westin chriswesti...@gmail.com
  wrote:
 
   No, but we should do something close to that.
  
   There are cases where the caller can handle the inability to get more
   memory, and may be able to go to disk. However, you are correct that
  there
   are many that can't handle an OOM, and that fail to check.
  
   Instead of unconditionally throwing OutOfMemoryRuntimeException, I
 would
   suggest that the buffer() call take a flag that indicates whether or
 not
  it
   should throw if it is unable to fulfill the request. This way, the call
   sites that can handle an OOM can pass in the flag to return null, and
 the
   rest can pass in the flag value to throw, and not have to have any
  checking
   code.
  
  
   On Tue, Jun 30, 2015 at 12:06 PM, Abdel Hakim Deneche 
   adene...@maprtech.com
wrote:
  
our current implementations of BufferAllocator.buffer(int, int)
 returns
null when it cannot allocate the buffer.
   
But looking through the code, there are many places that don't check
 if
   the
allocated buffer is null before trying to access it which will throw
 a
NullPointerException.
   
ValueVectors' allocateNewSafe() seem to be the only place that handle
  the
null in a specific manner.
   
Should we update the allocators' implementation to throw an
OutOfMemoryRuntimeException instead of returning null ? this has the
   added
benefit of displaying a proper out of memory error message to the
 user.
   
Thanks!
   
--
   
Abdelhakim Deneche
   
Software Engineer
   
  http://www.mapr.com/
   
   
Now Available - Free Hadoop On-Demand Training

   
  
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available

   
  
 
 
 
  --
 
  Abdelhakim Deneche
 
  Software Engineer
 
http://www.mapr.com/
 
 
  Now Available - Free Hadoop On-Demand Training
  
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available
  
 

Re: [DISCUSSION] should BufferAllocator.buffer() throw an OutOfMemoryException ?

[Chris Westin]

No, but we should do something close to that.

There are cases where the caller can handle the inability to get more
memory, and may be able to go to disk. However, you are correct that there
are many that can't handle an OOM, and that fail to check.

Instead of unconditionally throwing OutOfMemoryRuntimeException, I would
suggest that the buffer() call take a flag that indicates whether or not it
should throw if it is unable to fulfill the request. This way, the call
sites that can handle an OOM can pass in the flag to return null, and the
rest can pass in the flag value to throw, and not have to have any checking
code.


On Tue, Jun 30, 2015 at 12:06 PM, Abdel Hakim Deneche adene...@maprtech.com
 wrote:

 our current implementations of BufferAllocator.buffer(int, int) returns
 null when it cannot allocate the buffer.

 But looking through the code, there are many places that don't check if the
 allocated buffer is null before trying to access it which will throw a
 NullPointerException.

 ValueVectors' allocateNewSafe() seem to be the only place that handle the
 null in a specific manner.

 Should we update the allocators' implementation to throw an
 OutOfMemoryRuntimeException instead of returning null ? this has the added
 benefit of displaying a proper out of memory error message to the user.

 Thanks!

 --

 Abdelhakim Deneche

 Software Engineer

   http://www.mapr.com/


 Now Available - Free Hadoop On-Demand Training
 
 http://www.mapr.com/training?utm_source=Emailutm_medium=Signatureutm_campaign=Free%20available
 

Re: [DISCUSS] Allowing the option to use github pull requests in place of reviewboard

[Chris Westin]

And I'll bet GitHub is a lot faster too.

On Tue, Jun 23, 2015 at 2:23 PM, Hanifi Gunes hgu...@maprtech.com wrote:

 +1

 At the very least GitHub will be UP.

 On Tue, Jun 23, 2015 at 2:18 PM, Parth Chandra pchan...@maprtech.com
 wrote:

  +1 on trying this. RB has been pretty painful to us.
 
 
 
  On Mon, Jun 22, 2015 at 9:45 PM, Matthew Burgess mattyb...@gmail.com
  wrote:
 
   Is Travis https://travis-ci.org/  a viable option for the GitHub
  route?
   I
   use it for my own projects to build pull requests (with additional code
   quality targets like CheckStyle, PMD, etc.). Perhaps that would take
 some
   of
   the burden off the reviewers and let them focus on the proposed
   implementations, rather than some of the more tedious aspects of each
   review.
  
   From:  Jacques Nadeau jacq...@apache.org
   Reply-To:  dev@drill.apache.org
   Date:  Monday, June 22, 2015 at 10:22 PM
   To:  dev@drill.apache.org dev@drill.apache.org
   Subject:  Re: [DISCUSS] Allowing the option to use github pull requests
  in
   place of reviewboard
  
   I'm up for this if we deprecate the old way.  Having two different
   processes seems like overkill.  In general, I find the review interface
  of
   GitHub less expressive/clear but everything else is way better.
  
   On Mon, Jun 22, 2015 at 6:59 PM, Steven Phillips 
 sphill...@maprtech.com
  
   wrote:
  
 +1
   
 I am in favor of giving this a try.
   
 If I remember correctly, the reason we abandoned pull requests
   originally
 was because we couldn't close the pull requests through Github. A
   solution
 could be for whoever pushes the commit to the apache git repo to add
  the
 Line Closes request number. Github would then automatically
 close
   the
 pull request.
   
 On Mon, Jun 22, 2015 at 1:02 PM, Jason Altekruse 
   altekruseja...@gmail.com
 
 wrote:
   
  Hello Drill developers,
 
  I am writing this message today to propose allowing the use of
  github
 pull
  requests to perform reviews in place of the apache reviewboard
   instance.
 
  Reviewboard has caused a number of headaches in the past few
  months,
   and
 I
  think its time to evaluate the benefits of the apache
  infrastructure
  relative to the actual cost of using it in practice.
 
  For clarity of the discussion, we cannot use the complete github
 workflow.
  Comitters will still need to use patch files, or check out the
  branch
 used
  in the review request and push to apache master manually. I am
 not
  advocating for using a merging strategy with git, just for using
  the
 github
  web UI for reviews. I expect anyone generating a chain of commits
  as
  described below to use the rebasing workflow we do today.
   Additionally
 devs
  should only be breaking up work to make it easier to review, we
  will
   not
 be
  reviewing branches that contain a bunch of useless WIP commits.
 
  A few examples of problems I have experienced with reviewboard
   include:
  corruption of patches when they are downloaded, the web interface
   showing
  inconsistent content from the raw diff, and random rejection of
   patches
  that are based directly on the head of apache master.
 
  These are all serious blockers for getting code reviewed and
   integrated
  into the master branch in a timely manner.
 
  In addition to serious bugs in reviewboard, there are a number of
  difficulties with the combination of our typical dev workflow and
  how
  reviewboard works with patches. As we are still adding features
 to
   Drill,
  we often have several weeks of work to submit in response to a
 JIRA
   or
  series of related JIRAs. Sometimes this work can be broken up
 into
  independent reviewable units, and other times it cannot. When a
   series of
  changes requires a mixture of refactoring and additions, the
  process
   is
  currently quite painful. Ether reviewers need to look through a
  giant
 messy
  diff, or the submitters need to do a lot of extra work. This
   involves not
  only organizing their work into a reviewable series of commits,
 but
   also
  generating redundant squashed versions of the intermediate work
 to
   make
  reviewboard happy.
 
  For a relatively simple 3 part change, this involves creating 3
 reviewboard
  pages. The first will contain the first commit by itself. The
  second
   will
  have the first commits patch as a parent patch with the next
 change
   in
 the
  series uploaded as the core change to review. For the third
  change, a
  squashed version of the first two commits must be generated to
  serve
   as a
  parent patch and then the third changeset uploaded as the
  reviewable
  change. Frequently a change to the first commit requires
   regenerating all
  of these patches and uploading them to 

Re: Moving matured storage plugins out of contrib

[Chris Westin]

It's never been clear to me why these are in the same repo at all. If
someone wants to go off and build a plug-in, they don't really need to be
part of the project. Being part of the project brings a certain expectation
by users of support from developers, and Aditya's explanation basically
negates that.

On Tue, Jun 23, 2015 at 4:38 PM, Jason Altekruse altekruseja...@gmail.com
wrote:

 In light of Aditya's explanation I think this is a good way to clarify the
 relationship between Drill, Hive and Hbase. These are core plugins that we
 are planning to support completely, just as we do with the HDFS API in the
 FileSystemPlugin, which is currently a part of the exec module.

 +1

 On Tue, Jun 23, 2015 at 4:31 PM, Aditya adityakish...@gmail.com wrote:

  I have always interpreted contrib as an experimental code which are
  provided as is[1], not fully validated by the main project and possibly
  has not reached the same maturity as the rest of the project.
 
  It will help us in communicate the maturity of few plugins like hbase,
 hive
  and mongo as such while accepting more user contributions say cassandra,
  couchbase into Drill's code base.
 
  [1]
 
 http://stackoverflow.com/questions/7328852/what-are-apache-contrib-modules
 
  On Tue, Jun 23, 2015 at 2:33 PM, Jacques Nadeau jacq...@apache.org
  wrote:
 
   Unless we move contrib out of the build (and to a different repo), I'm
  not
   sure what the change really means.  Thoughts on this?
  
   On Tue, Jun 23, 2015 at 2:22 PM, Parth Chandra pchan...@maprtech.com
   wrote:
  
I'd be in favor of doing this in the 1.2 release cycle.
   
On Thu, Jun 18, 2015 at 6:30 PM, Aditya a...@apache.org wrote:
   
 Few of the storage plugins like HBase and Hive have matured enough
 to
   be
 moved out of contrib and into the mainline, probably under
exec/storage.

 If people think that it is time to do that, I can take this up.

 aditya...

   
  
 

[jira] [Created] (DRILL-3331) Provide the BufferManager at DrillBuf construction time

[Chris Westin (JIRA)]

Chris Westin created DRILL-3331:
---

 Summary: Provide the BufferManager at DrillBuf construction time
 Key: DRILL-3331
 URL: https://issues.apache.org/jira/browse/DRILL-3331
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Affects Versions: 1.1.0
Reporter: Chris Westin
Assignee: Chris Westin
Priority: Minor


DrillBuf.setBufferManager() allows for providing a buffer manager to handle 
reallocation and freeing by operators.

At this time, the BufferManager is not used by everything else yet. 
OperatorContextImpl has not yet been converted to use the BufferManager. Once 
it has been, then a DrillBuf either uses the BufferManager or it doesn't, and 
this argument can be surfaced on its constructors, and passed through 
(optionally null) from the allocator's buffer() call.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 34004: DRILL-1942: Improved memory allocator

[Chris Westin]

 that's excessive optimization, 
but you do think release(1)/retain(1) is?


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34004/#review88232
---


On June 16, 2015, 6:07 p.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34004/
 ---
 
 (Updated June 16, 2015, 6:07 p.m.)
 
 
 Review request for drill, Jacques Nadeau and Jason Altekruse.
 
 
 Bugs: DRILL-1942
 https://issues.apache.org/jira/browse/DRILL-1942
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Rewritten direct memory allocator. Simplified interface, and use, along with 
 a means to support additional allocation policies in the future. There are 
 features in the allocator and in DrillBuf that make finding leaks easier, as 
 well as better enforcement of limits. New features include transfer of 
 buffers, and better slicing support.
 
 This is a preliminary patch to get the review started because it touches a 
 lot of files (readers and record batches were made AutoCloseable in order to 
 cover cleanup). Subsequent reviews can use the differential view to just see 
 additional changes. The new allocator is in BaseAllocator.java (along with 
 derived classes RootAllocator and ChildAllocator); DrillBuf also has 
 significant changes. Most other changes in other files are just to use newer 
 interfaces, or to change cleanup() to close(), or to close subordinate 
 objects that are newly (Auto)Closeable. 1There are still a couple of things 
 to do:
 * Some TODO(cwestin)s to clean up tracing and debugging code, as well as 
 adding javadoc
 * Using the AllocatorOwner interface to replace the reallocation mechanism 
 for FragmentContext and OperatorContext so that the allocator doesn't know 
 anything about those objects.
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/AutoCloseablePointer.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillAutoCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/HistoricalLog.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/StackTrace.java 54068ec 
   common/src/main/java/org/apache/drill/common/config/DrillConfig.java 
 522303f 
   common/src/main/java/org/apache/drill/common/config/NestedConfig.java 
 3fd885f 
   
 contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseRecordReader.java
  9458db2 
   
 contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java
  3c8b9ba 
   
 contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoRecordReader.java
  182f5a4 
   exec/java-exec/src/main/codegen/templates/AbstractFieldWriter.java 1b5dad1 
   exec/java-exec/src/main/codegen/templates/BaseWriter.java ada410d 
   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 980f9ac 
   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 0dffa0b 
   exec/java-exec/src/main/codegen/templates/JsonOutputRecordWriter.java 
 ea643f0 
   exec/java-exec/src/main/codegen/templates/ListWriters.java ab78603 
   exec/java-exec/src/main/codegen/templates/MapWriters.java 06a6813 
   exec/java-exec/src/main/codegen/templates/NullableValueVectors.java ce6a3a7 
   exec/java-exec/src/main/codegen/templates/ParquetOutputRecordWriter.java 
 35777b0 
   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java 37b8fac 
   exec/java-exec/src/main/codegen/templates/StringOutputRecordWriter.java 
 f704cca 
   exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 
 529f21b 
   exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java 3ec6b3e 
   exec/java-exec/src/main/java/io/netty/buffer/FakeAllocator.java 721aff9 
   exec/java-exec/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java 
 2ca79f0 
   exec/java-exec/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java 
 e332b13 
   exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 
 8a24e8d 
   exec/java-exec/src/main/java/org/apache/drill/exec/TestMemoryRetention.java 
 37e5389 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/cache/AbstractStreamSerializable.java
  ef488d6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/cache/LoopedAbstractDrillSerializable.java
  d2a7458 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
  016cd92 
   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 
 c642c4a 
   exec/java-exec/src/main/java/org/apache/drill/exec/client/DumpCat.java 
 55d9cf3 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/client

Re: Review Request 34004: DRILL-1942: Improved memory allocator

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34004/#review88805
---



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 173)
https://reviews.apache.org/r/34004/#comment141388

Yes, and this will disappear after all tests work.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 187)
https://reviews.apache.org/r/34004/#comment141389

I didn't use an enum because it is possible to specify more than one flag 
at a time; they are then OR'ed together. I added javadoc for the flag that will 
survive the @Deprecated culling, but not for the one that is currently only 
used in other deprecated things.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 302)
https://reviews.apache.org/r/34004/#comment141391

Not everyone uses the BufferManager yet. Since the constructors are called 
from the allocator, the allocator interface will have to be changed to pass 
that through. Filed DRILL-3331, added TODO.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 312)
https://reviews.apache.org/r/34004/#comment141393

All that will change will be the deletion of the first two options, once 
everyone uses the BufferManager. The call can still fail in the same way if you 
try to ask for more space than is available.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 322)
https://reviews.apache.org/r/34004/#comment141397

That's what DRILL-3331, filed above, will eventually do. But so far only 
the FragmentContext has one. (I didn't make those changes, Jason did that a 
while ago.)



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 399)
https://reviews.apache.org/r/34004/#comment141398

No. The comment was a little out-of-date, so I fixed it to indicate that 
the buffer must use the newly provided ledger going forward.

Once you get to the allocator, you'll see that the ledger provides two 
things: (1) a private interface for the buffer to communicate with the 
allocator (there are no public implementations of the BufferLedger interface, 
so it must be provided by the allocator), and (2) there are multiple 
implementations of the ledgers. When a buffer gets shared, it gets a different 
ledger implementation. DrillBuf's only care is to use whatever one the 
allocator supplies gives to it. The allocator chooses which one is required.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 418)
https://reviews.apache.org/r/34004/#comment141399

The unfortunate lack of friend classes in Java makes it necessary to make 
this public. But the use of the BufferLedger interface means that no one else 
can call it, as there aren't any public implementations of that interface.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 568)
https://reviews.apache.org/r/34004/#comment141405

I'll do it before final push.



exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java (line 1022)
https://reviews.apache.org/r/34004/#comment141406

At this point, it is only used by the allocator for debugging assertions. 
However, the RPC code seems to depend on isRootBuffer for some reason, so I 
suspect it will want to use this instead.



exec/java-exec/src/main/java/io/netty/buffer/FakeAllocator.java (line 28)
https://reviews.apache.org/r/34004/#comment141411

Apparently not. Removed.


- Chris Westin


On June 16, 2015, 6:07 p.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34004/
 ---
 
 (Updated June 16, 2015, 6:07 p.m.)
 
 
 Review request for drill, Jacques Nadeau and Jason Altekruse.
 
 
 Bugs: DRILL-1942
 https://issues.apache.org/jira/browse/DRILL-1942
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Rewritten direct memory allocator. Simplified interface, and use, along with 
 a means to support additional allocation policies in the future. There are 
 features in the allocator and in DrillBuf that make finding leaks easier, as 
 well as better enforcement of limits. New features include transfer of 
 buffers, and better slicing support.
 
 This is a preliminary patch to get the review started because it touches a 
 lot of files (readers and record batches were made AutoCloseable in order to 
 cover cleanup). Subsequent reviews can use the differential view to just see 
 additional changes. The new allocator is in BaseAllocator.java (along with 
 derived classes RootAllocator and ChildAllocator); DrillBuf also has 
 significant changes. Most other changes in other files are just to use newer 
 interfaces, or to change cleanup() to close(), or to close subordinate 
 objects that are newly (Auto)Closeable. 1There

Re: Review Request 34004: DRILL-1942: Improved memory allocator

[Chris Westin]

 On June 18, 2015, 12:09 p.m., Sudheesh Katkam wrote:
  exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java,
   line 80
  https://reviews.apache.org/r/34004/diff/3/?file=983362#file983362line80
 
  Using ExecutionControlsInjector implies executionsControls is non-null. 
  On the latest master, there is a precondition check. 
  
  Why is this change necessary?

I ran into a case where sometimes one wasn't available. I can't remember the 
details now.


 On June 18, 2015, 12:09 p.m., Sudheesh Katkam wrote:
  exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java,
   line 74
  https://reviews.apache.org/r/34004/diff/3/?file=983386#file983386line74
 
  cancel() and isCancelled() need to be synchronized methods

Not necessary, because the variable is volatile. cancel() is already 
synchronized internally, but I changed to to be function-level to make it more 
obvious.


 On June 18, 2015, 12:09 p.m., Sudheesh Katkam wrote:
  exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java,
   line 216
  https://reviews.apache.org/r/34004/diff/3/?file=983430#file983430line216
 
  Can you use fail(...) instead?

Can you be more specific? Use fail() how? This function does a fair amount.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34004/#review87844
---


On June 16, 2015, 6:07 p.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34004/
 ---
 
 (Updated June 16, 2015, 6:07 p.m.)
 
 
 Review request for drill, Jacques Nadeau and Jason Altekruse.
 
 
 Bugs: DRILL-1942
 https://issues.apache.org/jira/browse/DRILL-1942
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Rewritten direct memory allocator. Simplified interface, and use, along with 
 a means to support additional allocation policies in the future. There are 
 features in the allocator and in DrillBuf that make finding leaks easier, as 
 well as better enforcement of limits. New features include transfer of 
 buffers, and better slicing support.
 
 This is a preliminary patch to get the review started because it touches a 
 lot of files (readers and record batches were made AutoCloseable in order to 
 cover cleanup). Subsequent reviews can use the differential view to just see 
 additional changes. The new allocator is in BaseAllocator.java (along with 
 derived classes RootAllocator and ChildAllocator); DrillBuf also has 
 significant changes. Most other changes in other files are just to use newer 
 interfaces, or to change cleanup() to close(), or to close subordinate 
 objects that are newly (Auto)Closeable. 1There are still a couple of things 
 to do:
 * Some TODO(cwestin)s to clean up tracing and debugging code, as well as 
 adding javadoc
 * Using the AllocatorOwner interface to replace the reallocation mechanism 
 for FragmentContext and OperatorContext so that the allocator doesn't know 
 anything about those objects.
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/AutoCloseablePointer.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillAutoCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/HistoricalLog.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/StackTrace.java 54068ec 
   common/src/main/java/org/apache/drill/common/config/DrillConfig.java 
 522303f 
   common/src/main/java/org/apache/drill/common/config/NestedConfig.java 
 3fd885f 
   
 contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseRecordReader.java
  9458db2 
   
 contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java
  3c8b9ba 
   
 contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoRecordReader.java
  182f5a4 
   exec/java-exec/src/main/codegen/templates/AbstractFieldWriter.java 1b5dad1 
   exec/java-exec/src/main/codegen/templates/BaseWriter.java ada410d 
   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 980f9ac 
   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 0dffa0b 
   exec/java-exec/src/main/codegen/templates/JsonOutputRecordWriter.java 
 ea643f0 
   exec/java-exec/src/main/codegen/templates/ListWriters.java ab78603 
   exec/java-exec/src/main/codegen/templates/MapWriters.java 06a6813 
   exec/java-exec/src/main/codegen/templates/NullableValueVectors.java ce6a3a7 
   exec/java-exec/src/main/codegen/templates/ParquetOutputRecordWriter.java 
 35777b0 
   exec/java-exec/src/main/codegen/templates

Re: Review Request 34004: DRILL-1942: Improved memory allocator

[Chris Westin]

,
   line 23
  https://reviews.apache.org/r/34004/diff/3/?file=983282#file983282line23
 
  Let's use the Stats infrastructure rather than introducing an JMX bean

I'll look into this for the next pass.


 On June 21, 2015, 9:06 p.m., Jacques Nadeau wrote:
  exec/java-exec/src/main/java/org/apache/drill/exec/memory/BaseAllocator.java,
   line 119
  https://reviews.apache.org/r/34004/diff/3/?file=983284#file983284line119
 
  Seems like the PerFragmentAllocationPolicy and Agent should be external 
  to the allocator.

I'm not sure about this -- I had some intuition that some policy 
implementations might need to be inner classes. I'll look into it for the next 
pass.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34004/#review88721
---


On June 16, 2015, 6:07 p.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34004/
 ---
 
 (Updated June 16, 2015, 6:07 p.m.)
 
 
 Review request for drill, Jacques Nadeau and Jason Altekruse.
 
 
 Bugs: DRILL-1942
 https://issues.apache.org/jira/browse/DRILL-1942
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Rewritten direct memory allocator. Simplified interface, and use, along with 
 a means to support additional allocation policies in the future. There are 
 features in the allocator and in DrillBuf that make finding leaks easier, as 
 well as better enforcement of limits. New features include transfer of 
 buffers, and better slicing support.
 
 This is a preliminary patch to get the review started because it touches a 
 lot of files (readers and record batches were made AutoCloseable in order to 
 cover cleanup). Subsequent reviews can use the differential view to just see 
 additional changes. The new allocator is in BaseAllocator.java (along with 
 derived classes RootAllocator and ChildAllocator); DrillBuf also has 
 significant changes. Most other changes in other files are just to use newer 
 interfaces, or to change cleanup() to close(), or to close subordinate 
 objects that are newly (Auto)Closeable. 1There are still a couple of things 
 to do:
 * Some TODO(cwestin)s to clean up tracing and debugging code, as well as 
 adding javadoc
 * Using the AllocatorOwner interface to replace the reallocation mechanism 
 for FragmentContext and OperatorContext so that the allocator doesn't know 
 anything about those objects.
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/AutoCloseablePointer.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillAutoCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/DrillCloseables.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/HistoricalLog.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/StackTrace.java 54068ec 
   common/src/main/java/org/apache/drill/common/config/DrillConfig.java 
 522303f 
   common/src/main/java/org/apache/drill/common/config/NestedConfig.java 
 3fd885f 
   
 contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseRecordReader.java
  9458db2 
   
 contrib/storage-hive/core/src/main/java/org/apache/drill/exec/store/hive/HiveRecordReader.java
  3c8b9ba 
   
 contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/MongoRecordReader.java
  182f5a4 
   exec/java-exec/src/main/codegen/templates/AbstractFieldWriter.java 1b5dad1 
   exec/java-exec/src/main/codegen/templates/BaseWriter.java ada410d 
   exec/java-exec/src/main/codegen/templates/ComplexWriters.java 980f9ac 
   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 0dffa0b 
   exec/java-exec/src/main/codegen/templates/JsonOutputRecordWriter.java 
 ea643f0 
   exec/java-exec/src/main/codegen/templates/ListWriters.java ab78603 
   exec/java-exec/src/main/codegen/templates/MapWriters.java 06a6813 
   exec/java-exec/src/main/codegen/templates/NullableValueVectors.java ce6a3a7 
   exec/java-exec/src/main/codegen/templates/ParquetOutputRecordWriter.java 
 35777b0 
   exec/java-exec/src/main/codegen/templates/RepeatedValueVectors.java 37b8fac 
   exec/java-exec/src/main/codegen/templates/StringOutputRecordWriter.java 
 f704cca 
   exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 
 529f21b 
   exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java 3ec6b3e 
   exec/java-exec/src/main/java/io/netty/buffer/FakeAllocator.java 721aff9 
   exec/java-exec/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java 
 2ca79f0 
   exec/java-exec/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java 
 e332b13 
   exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 
 8a24e8d 
   exec/java-exec/src/main/java/org/apache

[jira] [Created] (DRILL-3335) PrintingResultsListener's allocator should be a child of DrillClient's

[Chris Westin (JIRA)]

Chris Westin created DRILL-3335:
---

 Summary: PrintingResultsListener's allocator should be a child of 
DrillClient's
 Key: DRILL-3335
 URL: https://issues.apache.org/jira/browse/DRILL-3335
 Project: Apache Drill
  Issue Type: Bug
  Components: Client - CLI
Affects Versions: 1.1.0
Reporter: Chris Westin
Assignee: Daniel Barclay (Drill)


PrintingResultsListener's constructor creates a new top-level allocator. 
Instead, it should create ask DrillClient for a new child allocator that it 
should use. This could cause problems down the line, because we expect there to 
only be one top level allocator per node (or at least per JVM).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 35719: DRILL-3242: Update RPC layer so that requests and response are managed on a secondary thread.

[Chris Westin]

/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
 (line 517)
https://reviews.apache.org/r/35719/#comment141513

no blank lines here


- Chris Westin


On June 21, 2015, 5:40 p.m., Jacques Nadeau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/35719/
 ---
 
 (Updated June 21, 2015, 5:40 p.m.)
 
 
 Review request for drill, Chris Westin, Steven Phillips, and Sudheesh Katkam.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 - Add new SerializedExecutor (by cwestin) to manage serialized off-thread 
 executions
 - Create a separate serialized executor for fragment receiverFinished events.
 - Update serialized executor to pool object creation.
 - Ensure that FragmentExecutor acceptExternalEvents countdown occurs when 
 only execution is cancellation.
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/SerializedExecutor.java 
 PRE-CREATION 
   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 
 c642c4a 
   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 
 1cbe886 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 9ca09a1 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcConfig.java 
 ab6c375 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlClient.java
  159f1df 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java
  0cfa56e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlServer.java
  98ce9e1 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataClient.java 
 544bab9 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionCreator.java
  a76d753 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataConnectionManager.java
  8a947a9 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataResponseHandler.java
  721b83e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataRpcConfig.java
  c5cf498 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 
 80d2d6e 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java 
 b39a103 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java
  3f8122d 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 
 a197356 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java
  d0a998e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java
  6fdbfca 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/service/ServiceEngine.java 
 25ea307 
   exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 
 5939113 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  a9c2b6d 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestOptiqPlans.java
  dc37071 
 
 Diff: https://reviews.apache.org/r/35719/diff/
 
 
 Testing
 ---
 
 in progress
 
 
 Thanks,
 
 Jacques Nadeau
 

Build failure in Eclipse: Type Access restriction: The method 'VM.maxDirectMemory()' is not API

[Chris Westin]

I rebased a large set of changes against apache/master yesterday afternoon,
and now I can't build in Eclipse. I get the above warning in
DrillConfig.java, on this line:

  @SuppressWarnings(restriction)  private static final long
MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();


It builds fine from maven on the command-line, but not in eclipse (which is
preventing me from debugging a unit test).


When I mouse over the error indicator on the left-hand side of the file
editor, I see these:

Multiple markers at this line

- Unnecessary @SuppressWarnings(restriction)

- Access restriction: The method 'VM.maxDirectMemory()' is not API
(restriction on required library '/Library/Java/JavaVirtualMachines/

 jdk1.7.0_71.jdk/Contents/Home/jre/lib/rt.jar')

- Access restriction: The type 'VM' is not API (restriction on required
library '/Library/Java/JavaVirtualMachines/jdk1.7.0_71.jdk/Contents/Home/

  jre/lib/rt.jar')


There are no suggestions, and I'm not seeing anything obvious in web
searches. Any ideas on how to proceed?

Re: Build failure in Eclipse: Type Access restriction: The method 'VM.maxDirectMemory()' is not API

[Chris Westin]

Solved.

Removing and re-adding the JRE library as described here fixed it:
http://stackoverflow.com/questions/1089904/access-restriction-on-class-due-to-restriction-on-required-library/5746532#5746532
.  Weird, huh?


On Fri, Jun 19, 2015 at 8:02 AM, Chris Westin chriswesti...@gmail.com
wrote:

 I rebased a large set of changes against apache/master yesterday
 afternoon, and now I can't build in Eclipse. I get the above warning in
 DrillConfig.java, on this line:

   @SuppressWarnings(restriction)  private static final long
 MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();


 It builds fine from maven on the command-line, but not in eclipse (which
 is preventing me from debugging a unit test).


 When I mouse over the error indicator on the left-hand side of the file
 editor, I see these:

 Multiple markers at this line

 - Unnecessary @SuppressWarnings(restriction)

 - Access restriction: The method 'VM.maxDirectMemory()' is not API
 (restriction on required library '/Library/Java/JavaVirtualMachines/

  jdk1.7.0_71.jdk/Contents/Home/jre/lib/rt.jar')

 - Access restriction: The type 'VM' is not API (restriction on required
 library '/Library/Java/JavaVirtualMachines/jdk1.7.0_71.jdk/Contents/Home/

   jre/lib/rt.jar')


 There are no suggestions, and I'm not seeing anything obvious in web
 searches. Any ideas on how to proceed?

Re: Review Request 34004: DRILL-1942: Improved memory allocator

[Chris Westin]

/drill/exec/physical/impl/writer/TestWriter.java
 f4d505d 
  
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestLoad.java 
f57e765 
  
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java
 037c8c6 
  
exec/java-exec/src/test/java/org/apache/drill/exec/server/DrillClientFactory.java
 4230518 
  
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
 696aed8 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestDirectCodecFactory.java
 644144e 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/ischema/TestInfoSchemaFilterPushDown.java
 b6e789b 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java
 bb1af9e 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/FieldInfo.java 
34f60ba 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java
 8fdaa72 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java
 6326478 
  
exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestResourceLeak.java
 d7e317c 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/TestSplitAndTransfer.java
 4b3aa8a 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulation.java
 06a73e2 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java
 PRE-CREATION 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java
 d674d47 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestComplexTypeReader.java
 521a41d 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestComplexTypeWriter.java
 cb7bef2 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java
 912a5f0 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestRepeated.java
 6e2a2b5 
  exec/java-exec/src/test/resources/logback.xml 54ccb42 
  exec/java-exec/src/test/resources/logback.xml.sav PRE-CREATION 
  exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionImpl.java 
7c6ef7e 

Diff: https://reviews.apache.org/r/34004/diff/


Testing (updated)
---

mvn install passes
A few tests fail the presubmit suite, but all with the same 
IndexOutOfBoundsException. I've just gotten a reproducible case to run 
standalone on my laptop, so I'm debugging that.


Thanks,

Chris Westin

The weekly Drill hangout starts in five minutes

[Chris Westin]

https://plus.google.com/hangouts/_/event/ci4rdiju8bv04a64efj5fedd0lc

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34603/#review86801
---


Ship it (non-binding)

- Chris Westin


On June 2, 2015, 2:58 p.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34603/
 ---
 
 (Updated June 2, 2015, 2:58 p.m.)
 
 
 Review request for drill, Chris Westin, Jacques Nadeau, and Sudheesh Katkam.
 
 
 Bugs: DRILL-3167
 https://issues.apache.org/jira/browse/DRILL-3167
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 - In case of a failure the Foreman will cancel all fragments and move to a 
 FAILING state until all fragments are terminated
 - QueryManager.cancelExecutingFragments() returns false if no fragment 
 available
 - Web UI still displays FAILED when Foreman state is FAILING
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
  6656bf6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileWrapper.java
  dd26a76 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 5d07b49 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  71b77c6 
   exec/java-exec/src/main/resources/rest/profile/list.ftl cf92ede 
   exec/java-exec/src/main/resources/rest/profile/profile.ftl 46cdc83 
   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 
 92afa4f 
   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
 ---
 
 testing ...
 
 
 Thanks,
 
 abdelhakim deneche
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review86016
---


Ship it (non-binding)

- Chris Westin


On June 1, 2015, 10:54 a.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34829/
 ---
 
 (Updated June 1, 2015, 10:54 a.m.)
 
 
 Review request for drill, abdelhakim deneche, Chris Westin, and Jacques 
 Nadeau.
 
 
 Bugs: DRILL-3190
 https://issues.apache.org/jira/browse/DRILL-3190
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 DRILL-3190: Check for transitions from CANCELLATION_REQUESTED state
 + Moved state transition checks to QueryManager
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
  ceb77f0 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  71b77c6 
 
 Diff: https://reviews.apache.org/r/34829/diff/
 
 
 Testing
 ---
 
 Successful unit and regression tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34690/#review85847
---


SH-SH-SH-SHIP IT SH-SH-SH-SHIP IT (non-binding)


exec/java-exec/src/test/java/org/apache/drill/exec/testing/Controls.java
https://reviews.apache.org/r/34690/#comment137685

Oh geez, I can't believe setLength() isn't fluent like everything else on 
there. But leave it that way.


- Chris Westin


On May 29, 2015, 4:52 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34690/
 ---
 
 (Updated May 29, 2015, 4:52 p.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 to tests in TestDrillbitResilience
 + Added RepeatTestRule to tests that are flaky
 + Added Controls.Builder to create controls string in tests
 + Added @Ignore to failing tests (filed JIRAs)
 
 Other fixes:
 + Added @Override to ScanBatch#close to avoid potential bugs
 + Added docs link in ProtobufLengthDecoder
 + Fixed logging issue in CountDownLatchImpl
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/util/TestTools.java 5be8d40 
   common/src/test/java/org/apache/drill/test/DrillTest.java bbe014f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
  6176f77 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/ProtobufLengthDecoder.java
  4f075d3 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java
  16b9b63 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
  561d816 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
  639802f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
  8ee7d38 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 5d07b49 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  71b77c6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  e5e0700 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
  696aed8 
   exec/java-exec/src/test/java/org/apache/drill/exec/testing/Controls.java 
 PRE-CREATION 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java
  346c6dd 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  c98f54c 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java
  e3558a1 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  ba29c58 
 
 Diff: https://reviews.apache.org/r/34690/diff/
 
 
 Testing
 ---
 
 Unit tests pass on Linux, and successful regression tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34690/#review85816
---


A couple of minor nits, but otherwise SHIP IT (non-binding).


exec/java-exec/src/test/java/org/apache/drill/exec/testing/Controls.java
https://reviews.apache.org/r/34690/#comment137638

This might be a bit misleading; it's not partial, it's the same builder.

I'd just say this builder to indicate that this object is meant to be 
used in a fluent style (that's how this kind of builder object is often 
described). As it is now, it could imply the builders are also immutable, and 
that it returns a new copy with the added exception injection.



exec/java-exec/src/test/java/org/apache/drill/exec/testing/Controls.java
https://reviews.apache.org/r/34690/#comment137636

I'll bet it's cheaper to use

builder.setLength(builder.length() - 1)

because it won't be set up with the expectation that there are following 
characters to copy back one space.


- Chris Westin


On May 29, 2015, 10:48 a.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34690/
 ---
 
 (Updated May 29, 2015, 10:48 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 to tests in TestDrillbitResilience
 + Added RepeatTestRule to tests that are flaky
 + Added Controls.Builder to create controls string in tests
 + Added @Ignore to failing tests (filed JIRAs)
 
 Other fixes:
 + Added @Override to ScanBatch#close to avoid potential bugs
 + Added docs link in ProtobufLengthDecoder
 + Fixed logging issue in CountDownLatchImpl
 
 
 Diffs
 -
 
   common/src/main/java/org/apache/drill/common/util/RepeatTestRule.java 
 PRE-CREATION 
   common/src/main/java/org/apache/drill/common/util/TestTools.java 5be8d40 
   common/src/test/java/org/apache/drill/test/DrillTest.java bbe014f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
  6176f77 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/ProtobufLengthDecoder.java
  4f075d3 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java
  16b9b63 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
  561d816 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
  639802f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
  8ee7d38 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 5d07b49 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  71b77c6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  e5e0700 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
  696aed8 
   exec/java-exec/src/test/java/org/apache/drill/exec/testing/Controls.java 
 PRE-CREATION 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java
  346c6dd 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  c98f54c 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java
  e3558a1 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  ba29c58 
 
 Diff: https://reviews.apache.org/r/34690/diff/
 
 
 Testing
 ---
 
 Unit tests pass on Linux, and successful regression tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34829/#review85819
---


One minor optimization, then SHIP IT (non-binding)


exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
https://reviews.apache.org/r/34829/#comment137641

This variable isn't used until later, so move it to after the if block so 
it isn't needlessly computed.


- Chris Westin


On May 29, 2015, 1:44 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34829/
 ---
 
 (Updated May 29, 2015, 1:44 p.m.)
 
 
 Review request for drill, abdelhakim deneche, Chris Westin, and Jacques 
 Nadeau.
 
 
 Bugs: DRILL-3190
 https://issues.apache.org/jira/browse/DRILL-3190
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 DRILL-3190: Check for transition from CANCELLATION_REQUESTED to non-terminal 
 state in FragmentData
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/FragmentData.java
  ceb77f0 
 
 Diff: https://reviews.apache.org/r/34829/diff/
 
 
 Testing
 ---
 
 Running unit and regresssion tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

Re: Please don't use assert in unit tests

[Chris Westin]

In this context, I don't see a difference between test code, and test setup
code. If you run the test without assertions enabled, and it appears to
pass as a result, you've been misled (this is what got me started down this
path). Setup failure may amount to the test doing nothing, not running, or
only partially testing what it was meant to.

Quite simply: the result of running the test should be the same whether
Java assertions are enabled or not. result here includes not just any
textual output, but execution of what the test actually does.

Note that this may imply that some non-test code should also not be using
Java assert. For example, one problem I ran into was that nullable value
vectors' implementation of get() used assert !isNull(). This in turn led
to the test (in TestValueVector) actually checked for AssertionError to be
thrown, but only if assertions were enabled. In this case, the test failed
if assertions were NOT enabled. I fixed ValueVectors (in a previous patch)
to throw an IllegalArgumentException if you try to get a value that is null
-- something which could have also misled users badly by returning an
uninitialized value for something that should have been null, if there were
a bug elsewhere in the code that didn't check for null first.

I'm beginning to think we should go the Google route and not allow Java
assert anywhere, instead always throwing real exceptions (possibly
indirectly by using the Preconditions package). But I'm not quite ready to
go there yet. For now I'll stick to asking that we not use Java assert in
tests and test support code.



On Thu, May 28, 2015 at 11:12 AM, Jason Altekruse altekruseja...@gmail.com
wrote:

 Hello Drillers,

 I merged a patch from Chris yesterday that cleaned up and fixed some issues
 in a good number of the unit tests.

 The primary problem he solved was improper usage of Java asserts to test
 for the detecting success conditions in the tests. There was no single
 contributor or component that they were added by (several of them were from
 my own tests), so I would just like to remind everyone that there should be
 no expectation that assertions are enabled for proper functionality, even
 in the unit tests. As is now the case throughout the code, please use JUnit
 assert functions (assertTrue, assertEquals, etc.) for testing
 success/failure conditions in tests.

 While I did review the patch before I merged it, Daniel noticed that some
 of the asserts that were refactored were not actually designed for checking
 Drill code/functionality, but had been added to indicate improper test
 setup. In these cases he had deliberately added Java asserts to
 differentiate these two cases.

 He is fine if we want to ban Java asserts from tests, but would like to
 have a discussion about possible ways to differentiate checks for test
 setup correctness from actual test failures. The two options I can think of
 are allowing java asserts back into the unit tests, but only to test issues
 with test setup. The other is to add a standard around messages to
 differentiate these two cases if we use the Junit assert methods for both
 of these cases. Please respond with your objections or approval of either
 of these methods. Once we have a good consensus I will be adding this to
 the contribution guide on the drill docs.


 On Thu, Apr 30, 2015 at 7:50 PM, Chris Westin chriswesti...@gmail.com
 wrote:

  I had some confusing times this afternoon as I was working through unit
  tests to validate the new allocator I've been working on. (Yes,
  TopLevelAllocator will be replaced soon; and the new allocator found
 leaks
  that were happening before.)
 
  By default, when running a junit test in my IDE (eclipse, but I'm sure
 this
  is true elsewhere), it gets no JVM arguments. It's possible to create a
  test profile that includes JVM arguments, and this is what is required if
  you want to supply things like -ea (enable assertions in the JVM).
 
  This afternoon I was fooled by some tests that appeared to work when I
 ran
  them. This was because they checked their results with java assert,
 instead
  of with the org.junit.Assert.assertXXX static functions, and I didn't
 have
  -ea. (Yes, I've fixed this in my current branch, and replaced the asserts
  with the appropriate junit assertXXX()s.)
 
  So, don't use assert to check test results (or verify state in helpers)
 in
  unit tests. This can badly mislead someone who's running the tests into
  thinking they are working even if they aren't.
 
  And BTW, I also spotted some tests that were using the junit's
 assertEquals
  like this:  assertEquals(true, boolean-condition). Just FYI, for things
  like this, you can use assertTrue(boolean-condition). There's also an
  assertFalse(). Check the javadoc if you need something beyond
  assertEquals() -- there are other options.
 
  Thank you for observing all safety precautions.
  Chris
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review85205
---


Ship it (non-binding)

- Chris Westin


On May 22, 2015, 4:15 p.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34541/
 ---
 
 (Updated May 22, 2015, 4:15 p.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-3147
 https://issues.apache.org/jira/browse/DRILL-3147
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
 - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because 
 it requires a large dataset
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 
 80d2d6e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
  11b6cc8 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
  b770a33 
   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java a07f621 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
  PRE-CREATION 
   exec/java-exec/src/test/resources/tpcds-sf1/q73.sql PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/34541/diff/
 
 
 Testing
 ---
 
 still need to run the tests!
 
 
 Thanks,
 
 abdelhakim deneche
 

Re: Review Request 34598: DRILL-2923: Ensure all unit tests pass without assertions enabled

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34598/
---

(Updated May 26, 2015, 10:39 a.m.)


Review request for drill and Jason Altekruse.


Changes
---

Added testing information, and added Jason as a reviewer.


Bugs: DRILL-2923
https://issues.apache.org/jira/browse/DRILL-2923


Repository: drill-git


Description (updated)
---

Modified a number of unit tests not to use java assert, but to instead use one 
of junit's assertTrue(), assertFalse(), or some other form. Modified test 
support code that used asserts to throw IllegalStateExceptions instead.


Diffs
-

  common/src/test/java/org/apache/drill/common/expression/PathSegmentTests.java 
07b2385 
  
contrib/storage-hive/core/src/test/java/org/apache/drill/exec/TestHivePartitionPruning.java
 7353e05 
  exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java f909681 
  
exec/java-exec/src/test/java/org/apache/drill/exec/compile/bytecode/ReplaceMethodInvoke.java
 bc2d929 
  
exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/ExpressionInterpreterTest.java
 2a83a53 
  
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/mergereceiver/TestMergingReceiver.java
 0122c08 
  
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/sort/TestSimpleSort.java
 f37624a 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/ischema/TestInfoSchemaFilterPushDown.java
 b6e789b 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/FieldInfo.java 
34f60ba 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java
 8fdaa72 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java
 6326478 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java
 d674d47 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestRepeated.java
 6e2a2b5 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/DatabaseMetaDataGetColumnsTest.java
 a6c2da8 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/ResultSetGetMethodConversionsTest.java
 4ad80d1 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/proxy/TracingProxyDriverTest.java 
389cbac 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2128GetColumnsDataTypeNotTypeCodeIntBugsTest.java
 4203c4a 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2461IntervalsBreakInfoSchemaBugTest.java
 f0a9eb0 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2463GetNullsFailedWithAssertionsBugTest.java
 c355142 
  exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java 4081696 

Diff: https://reviews.apache.org/r/34598/diff/


Testing (updated)
---

mvn install, both with and without assertions enabled (via the root pom.xml's 
surefire args)
presubmit suite
* Regression and Customer fails with the known execHive.sh issues
* TPCH SF100 passes


Thanks,

Chris Westin

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

[Chris Westin]

 On May 22, 2015, 11:15 a.m., Chris Westin wrote:
  exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java,
   line 56
  https://reviews.apache.org/r/34541/diff/3/?file=966939#file966939line56
 
  Can we indent this a little better so it's easier to see the nested 
  select separately from the rest of the query, as well as the order by and 
  group by clauses? It might help to condense the select lists onto a single 
  line. For example
  
  select .,
 more columns, if necessary
  from ...
 more of that, if necessary
  where ...
  group by ...
  order by ...
  limit ...
 
 Sudheesh Katkam wrote:
 You could put the query in a file and use BaseTestQuery.getFile(...)
 For example: 
 ```
 final String query = BaseTestQuery.getFile(queries/tpch/09.sql);
 ```

Oh yeah, I like that even better, because then you can format the query nicely 
without having a bunch of Java String muck (+, etc) in the way.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84943
---


On May 21, 2015, 12: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, 12:34 p.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-3147
 https://issues.apache.org/jira/browse/DRILL-3147
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
 - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because 
 it requires a large dataset
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 
 80d2d6e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
  11b6cc8 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
  b770a33 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/34541/diff/
 
 
 Testing
 ---
 
 still need to run the tests!
 
 
 Thanks,
 
 abdelhakim deneche
 

Review Request 34598: DRILL-2923: Ensure all unit tests pass without assertions enabled

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34598/
---

Review request for drill.


Bugs: DRILL-2923
https://issues.apache.org/jira/browse/DRILL-2923


Repository: drill-git


Description
---

Modified a number of unit tests not to use assert, but to instead use one of 
junit's assertTrue(), assertFalse(), or some other form. Modified test support 
code that used asserts to throw IllegalStateExceptions instead.


Diffs
-

  common/src/test/java/org/apache/drill/common/expression/PathSegmentTests.java 
07b2385 
  
contrib/storage-hive/core/src/test/java/org/apache/drill/exec/TestHivePartitionPruning.java
 7353e05 
  exec/java-exec/src/test/java/org/apache/drill/PlanTestBase.java f909681 
  
exec/java-exec/src/test/java/org/apache/drill/exec/compile/bytecode/ReplaceMethodInvoke.java
 bc2d929 
  
exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/ExpressionInterpreterTest.java
 2a83a53 
  
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/mergereceiver/TestMergingReceiver.java
 0122c08 
  
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/sort/TestSimpleSort.java
 f37624a 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/ischema/TestInfoSchemaFilterPushDown.java
 b6e789b 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/FieldInfo.java 
34f60ba 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java
 8fdaa72 
  
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java
 6326478 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/fn/TestJsonReaderWithSparseFiles.java
 d674d47 
  
exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestRepeated.java
 6e2a2b5 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/DatabaseMetaDataGetColumnsTest.java
 a6c2da8 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/ResultSetGetMethodConversionsTest.java
 4ad80d1 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/proxy/TracingProxyDriverTest.java 
389cbac 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2128GetColumnsDataTypeNotTypeCodeIntBugsTest.java
 4203c4a 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2461IntervalsBreakInfoSchemaBugTest.java
 f0a9eb0 
  
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2463GetNullsFailedWithAssertionsBugTest.java
 c355142 
  exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java 4081696 

Diff: https://reviews.apache.org/r/34598/diff/


Testing
---

mvn install, both with and without assertions enabled (via the root pom.xml's 
surefire args)
presubmit suite running


Thanks,

Chris Westin

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34541/#review84943
---


Steven knows much more about this area of the code than I do, so my comments 
are mostly stylistic, with one exception.


exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java
https://reviews.apache.org/r/34541/#comment136395

Can we make this final?



exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
https://reviews.apache.org/r/34541/#comment136399

Nothing else in this file is synchronized, so what is this protecting?



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
https://reviews.apache.org/r/34541/#comment136401

final



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
https://reviews.apache.org/r/34541/#comment136402

Can we indent this a little better so it's easier to see the nested select 
separately from the rest of the query, as well as the order by and group by 
clauses? It might help to condense the select lists onto a single line. For 
example

select .,
   more columns, if necessary
from ...
   more of that, if necessary
where ...
group by ...
order by ...
limit ...



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
https://reviews.apache.org/r/34541/#comment136403

Spaces around the + operator, please.



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
https://reviews.apache.org/r/34541/#comment136404

Can we move this to BaseTestQuery and make it protected? Other tests could 
use this.


- Chris Westin


On May 21, 2015, 12: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, 12:34 p.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-3147
 https://issues.apache.org/jira/browse/DRILL-3147
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 - BaseRawBatchBuffer methods enqueue() and kill() are now synchronized
 - TestTpcdsSf1Leak test reproduces the leak, it's ignored by default because 
 it requires a large dataset
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataServer.java 
 80d2d6e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/BaseRawBatchBuffer.java
  11b6cc8 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
  b770a33 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestTpcdsSf1Leaks.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/34541/diff/
 
 
 Testing
 ---
 
 still need to run the tests!
 
 
 Thanks,
 
 abdelhakim deneche
 

Re: Review Request 34074: DRILL-3035 Create ControlsInjector interface to enforce implementing methods

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34074/#review84941
---


Ship it (non-binding).


exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
https://reviews.apache.org/r/34074/#comment136391

Good change going to ImmutableList.


- Chris Westin


On May 21, 2015, 3:07 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34074/
 ---
 
 (Updated May 21, 2015, 3:07 p.m.)
 
 
 Review request for drill, Chris Westin and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 [DRILL-3035](https://issues.apache.org/jira/browse/DRILL-3035): Created 
 ControlsInjector interface to enforce method implementations
 + [DRILL-2867](https://issues.apache.org/jira/browse/DRILL-2867): Add 
 ControlsValidator to VALIDATORS only if assertions are enabled
 + return in ExecutionControls ctor if assertions are not enabled
 + added InjectorFactory class to align with the logger pattern
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
  e2d5b18 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/ops/AccountingDataTunnel.java
  2659464 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
  6176f77 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
  76dc91c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java
  1f6767c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
  baf9bda 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
  e210514 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java
  684f715 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
  8871a5f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
  e385600 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/data/DataTunnel.java 
 7a6477e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
  e268e64 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java
  a893da1 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ControlsInjector.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ControlsInjectorFactory.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
  639802f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java
  e3a4ba6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
  bf4221e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 5d07b49 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  e5e0700 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  c98f54c 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java
  e3558a1 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  ba29c58 
 
 Diff: https://reviews.apache.org/r/34074/diff/
 
 
 Testing
 ---
 
 Currently running unit tests and regression tests
 
 
 Thanks,
 
 Sudheesh Katkam
 

[jira] [Created] (DRILL-3154) Coding standards and code cleanup

[Chris Westin (JIRA)]

Chris Westin created DRILL-3154:
---

 Summary: Coding standards and code cleanup
 Key: DRILL-3154
 URL: https://issues.apache.org/jira/browse/DRILL-3154
 Project: Apache Drill
  Issue Type: Bug
Affects Versions: 1.0.0
Reporter: Chris Westin
Assignee: Chris Westin
 Fix For: Future


This is an umbrella ticket for a number of tasks we'd like to take on wrt 
cleaning up the code base. This includes coming up with a little more stringent 
coding standard, and enforcing it via checkstyle, and getting rid of warnings, 
and making those cause compilation errors so that we keep them out of the code 
base. There are other things along the way, such as making the loggers private 
(already agreed to on the dev list), and making more use of AutoCloseable and 
Closeable to help us detect the use of resources that need to be cleaned up (an 
area we've seen a lot of trouble with because of the use of direct memory).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-3139) Query against yelp academic dataset causes exception

[Chris Westin (JIRA)]

Chris Westin created DRILL-3139:
---

 Summary: Query against yelp academic dataset causes exception
 Key: DRILL-3139
 URL: https://issues.apache.org/jira/browse/DRILL-3139
 Project: Apache Drill
  Issue Type: Bug
  Components: Client - CLI
Affects Versions: 0.9.0
 Environment: OSX
Reporter: Chris Westin
Assignee: Daniel Barclay (Drill)


I was following along the tutorial for Analyzing the Yelp Academic Dataset.
I tried the first query Querying Yelp Business Data WITHOUT the limit clause:
select * from
 dfs.`/users/nrentachintala/Downloads/yelp/yelp_academic_dataset_business.json`;

(Adjust for your own download path).

A bunch of data comes out, followed by an exception:
Dietary Restrictions:{}} | business   | null  |
java.lang.RuntimeException: java.sql.SQLException: Failure while executing 
query.
at sqlline.SqlLine$IncrementalRows.hasNext(SqlLine.java:2514)
at sqlline.SqlLine$TableOutputFormat.print(SqlLine.java:2148)
at sqlline.SqlLine.print(SqlLine.java:1809)
at sqlline.SqlLine$Commands.execute(SqlLine.java:3766)
at sqlline.SqlLine$Commands.sql(SqlLine.java:3663)
at sqlline.SqlLine.dispatch(SqlLine.java:889)
at sqlline.SqlLine.begin(SqlLine.java:763)
at sqlline.SqlLine.start(SqlLine.java:498)
at sqlline.SqlLine.main(SqlLine.java:460)
0: jdbc:drill:zk=local

Note that this was tried against the 0.9.0 tarball referred to in the tutorials 
and download links.




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Review Request 34297: DRILL-3103: allow write activity to also count against not timing out in the RPC layer

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34297/
---

Review request for drill.


Bugs: DRILL-3103
https://issues.apache.org/jira/browse/DRILL-3103


Repository: drill-git


Description
---

Modified BasicServer so that it also bumps an activity counter for writes as 
well as reads when figuring out when to time out.


Diffs
-

  exec/java-exec/src/main/java/org/apache/drill/exec/rpc/BasicServer.java 
5c04264 

Diff: https://reviews.apache.org/r/34297/diff/


Testing
---

Tests running


Thanks,

Chris Westin

Re: Review Request 34297: DRILL-3103: allow write activity to also count against not timing out in the RPC layer

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34297/
---

(Updated May 15, 2015, 6 p.m.)


Review request for drill and Jacques Nadeau.


Changes
---

WriteActivityHandler.write() change to use ChannelHandlerContext.write() for 
forward its message along the handler chain; this shouldn't make a difference 
over using super., because the default implementation of 
ChannelOutboundHandlerAdapter is supposedly already doing that, but it is the 
recommended thing to do according to 
http://netty.io/4.0/api/io/netty/channel/ChannelPipeline.html . More 
significantly, to combat our concern that the write activity handler might not 
be called when we want it to, changed it's pipeline add to addFirst() (instead 
of addLast()). Since it forwards it's calls, that should be harmless, but 
ensures that it gets called even if some other upstream handler handles the 
event and doesn't forward it.


Bugs: DRILL-3103
https://issues.apache.org/jira/browse/DRILL-3103


Repository: drill-git


Description
---

Modified BasicServer so that it also bumps an activity counter for writes as 
well as reads when figuring out when to time out.


Diffs (updated)
-

  exec/java-exec/src/main/java/org/apache/drill/exec/rpc/BasicServer.java 
5c04264 

Diff: https://reviews.apache.org/r/34297/diff/


Testing
---

mvn install passed
presubmit suite running


Thanks,

Chris Westin

Re: Review Request 34297: DRILL-3103: allow write activity to also count against not timing out in the RPC layer

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34297/
---

(Updated May 15, 2015, 5:18 p.m.)


Review request for drill.


Bugs: DRILL-3103
https://issues.apache.org/jira/browse/DRILL-3103


Repository: drill-git


Description
---

Modified BasicServer so that it also bumps an activity counter for writes as 
well as reads when figuring out when to time out.


Diffs
-

  exec/java-exec/src/main/java/org/apache/drill/exec/rpc/BasicServer.java 
5c04264 

Diff: https://reviews.apache.org/r/34297/diff/


Testing (updated)
---

mvn install passed
presubmit suite running


Thanks,

Chris Westin

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34245/
---

Review request for drill and Jacques Nadeau.


Bugs: DRILL-3063
https://issues.apache.org/jira/browse/DRILL-3063


Repository: drill-git


Description
---

Changed the cleanup handling at the end of ImplCreator.getExec(), and
handle the newly returned null value in FragmentExecutor.run().


Diffs
-

  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
 77ca0f5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
 8c49d68 

Diff: https://reviews.apache.org/r/34245/diff/


Testing
---

tests are running


Thanks,

Chris Westin

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

[Chris Westin]

 On May 14, 2015, 5:33 p.m., abdelhakim deneche wrote:
  exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java,
   line 230
  https://reviews.apache.org/r/34245/diff/1/?file=960805#file960805line230
 
  if we return here, this fragment will never send it's final state to 
  the Foreman

We've already marked the context as failed. Won't notifying the Foreman get 
taken care of in the finally clause below (the return is in the try)? It seemed 
to work when we tested this as the fix for a memory leak.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34245/#review83879
---


On May 14, 2015, 5:10 p.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34245/
 ---
 
 (Updated May 14, 2015, 5:10 p.m.)
 
 
 Review request for drill and Jacques Nadeau.
 
 
 Bugs: DRILL-3063
 https://issues.apache.org/jira/browse/DRILL-3063
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Changed the cleanup handling at the end of ImplCreator.getExec(), and
 handle the newly returned null value in FragmentExecutor.run().
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
  77ca0f5 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  8c49d68 
 
 Diff: https://reviews.apache.org/r/34245/diff/
 
 
 Testing
 ---
 
 tests are running
 
 
 Thanks,
 
 Chris Westin
 

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34245/
---

(Updated May 14, 2015, 8:07 p.m.)


Review request for drill and Jacques Nadeau.


Changes
---

just updated Testing Done


Bugs: DRILL-3063
https://issues.apache.org/jira/browse/DRILL-3063


Repository: drill-git


Description
---

Changed the cleanup handling at the end of ImplCreator.getExec(), and
handle the newly returned null value in FragmentExecutor.run().


Diffs
-

  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
 77ca0f5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
 8c49d68 

Diff: https://reviews.apache.org/r/34245/diff/


Testing (updated)
---

I've run the presubmit suite twice, and it has failed differently both times.
The first time, regression failed in 
/root/private-sql-hadoop-test/framework/resources/Precommit/Functional/partition_pruning/hive/text/hier_intstring/data/textSelectMultipleAnd.q
The second time, regression failed in 
/root/private-sql-hadoop-test/framework/resources/Functional/Passing/complex/json/complex51.q


Thanks,

Chris Westin

[jira] [Created] (DRILL-3063) TestQueriesOnLargeFile leaks memory with 16M limit

[Chris Westin (JIRA)]

Chris Westin created DRILL-3063:
---

 Summary: TestQueriesOnLargeFile leaks memory with 16M limit
 Key: DRILL-3063
 URL: https://issues.apache.org/jira/browse/DRILL-3063
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Reporter: Chris Westin
Assignee: Chris Westin
 Fix For: 1.0.0


I ran the TestQueriesOnLargeFile unit test with a limited memory environment, 
limiting direct memory to 16M. At the end of the test, the shutdown hook 
reports a memory leak.

Here is the test launch configuration:
-Xms512m
-Xmx3g
-Ddrill.exec.http.enabled=false
-Ddrill.exec.sys.store.provider.local.write=false
-Dorg.apache.drill.exec.server.Drillbit.system_options=org.apache.drill.exec.compile.ClassTransformer.scalar_replacement=on
-XX:MaxPermSize=256M -XX:MaxDirectMemorySize=3072M
-XX:+CMSClassUnloadingEnabled -ea
-Ddrill.exec.memory.top.max=16777216

Here's what I see at the end:
at 
org.apache.drill.exec.server.Drillbit$ShutdownThread.run(Drillbit.java:333)
Caused by: java.lang.IllegalStateException: Failure while closing accountor.  
Expected private and shared pools to be set to initial values.  However, one or 
more were not.  Stats are
zoneinitallocated   delta 
private 0   0   0 
shared  1677721613777216300.
at 
org.apache.drill.exec.memory.AtomicRemainder.close(AtomicRemainder.java:200)
at org.apache.drill.exec.memory.Accountor.close(Accountor.java:386)
at 
org.apache.drill.exec.memory.TopLevelAllocator.close(TopLevelAllocator.java:171)
at 
org.apache.drill.exec.server.BootStrapContext.close(BootStrapContext.java:75)
at com.google.common.io.Closeables.close(Closeables.java:77)
at com.google.common.io.Closeables.closeQuietly(Closeables.java:108)
at org.apache.drill.exec.server.Drillbit.close(Drillbit.java:292)
at 
org.apache.drill.exec.server.Drillbit$ShutdownThread.run(Drillbit.java:330)




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34173/#review83655
---


A couple minor things, and a suggestion on how to get even more out of this 
test by testing the same queries with failures at multiple different locations.


exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
https://reviews.apache.org/r/34173/#comment134664

There could be more than one injection site, so this should have a more 
specific name.



exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
https://reviews.apache.org/r/34173/#comment134665

It took me a minute to understand this; it is an unusual way to use the 
exception injector, so please add a comment explaining what's going on here so 
others that aren't as familiar with it or the allocator (they might have 
assumed this should be throwing OutOfMemoryException) will understand why you 
catch the exception and return null.



exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java
https://reviews.apache.org/r/34173/#comment134666

Exception{ = Exception { throughout the file.



exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java
https://reviews.apache.org/r/34173/#comment134667

Could this method be put into BaseTestQuery so that other tests could use 
it? Maybe it should be TestWithOutOfMemoryException then.



exec/java-exec/src/test/java/org/apache/drill/TestAllocationException.java
https://reviews.apache.org/r/34173/#comment134668

I might parameterize the nSkip value, so that these queries could be tested 
with this exception being thrown in different places.

For example, we might define a fixed set of numbers like

final int nSkips[] = {200, 250, 300, 375};

testWithException takes a single number as an additional argument.

testWithExeptions might take an array, and call testWithException in a loop 
with each of the values.

Then we could test each query with failures at different locations. As 
above, these might be useful in BaseTestQuery, but if that's too hard for now, 
just do it here.


- Chris Westin


On May 13, 2015, 10:54 a.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34173/
 ---
 
 (Updated May 13, 2015, 10:54 a.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-3053
 https://issues.apache.org/jira/browse/DRILL-3053
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 added an unchecked injection site in ChildAllocator.buffer(int, int). It's 
 only enabled when assertions are enabled.
 You can throw any unchecked or Error exception, but if you throw a 
 NullPointerException the buffer() method will just return null without 
 throwing any exception to simulate a normal allocation failure
 
 
 Diffs
 -
 
   
 common/src/main/java/org/apache/drill/common/exceptions/DrillRuntimeException.java
  abc7065 
   common/src/main/java/org/apache/drill/common/exceptions/UserException.java 
 a67cb3f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
  9670c7e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/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: can't build - Maven / Calcite / repository error

[Chris Westin]

If this is the public build, there shouldn't be any secure repos involved.

On Wed, May 13, 2015 at 1:03 PM, Hanifi Gunes hgu...@maprtech.com wrote:

 *hostname in certificate didn't match: n136.network-auth.com
 http://n136.network-auth.com/ != *.meraki.com http://meraki.com/ OR
 *.meraki.com http://meraki.com/ OR meraki.com http://meraki.com/
 -*

 Sounds like a network issue. If you are under a corporate network,
 disconnect and reconnect. Make sure you can access to secure repo where the
 dependency is deployed.


 On Wed, May 13, 2015 at 12:56 PM, Daniel Barclay dbarc...@maprtech.com
 wrote:

  Mehant,
 
  I've having trouble building from master plus DRILL-2993 and DRILL-3020
  changes.  Maven has some kind of trouble getting Calcite:
 
 
  [ERROR] Failed to execute goal on project drill-common: Could not resolve
  dependencies for project
 org.apache.drill:drill-common:jar:1.0.0-SNAPSHOT:
  Failed to collect dependencies at
  org.apache.calcite:calcite-core:jar:1.1.0-drill-r7: Failed to read
 artifact
  descriptor for org.apache.calcite:calcite-core:jar:1.1.0-drill-r7: Could
  not transfer artifact org.apache.calcite:calcite-core:pom:1.1.0-drill-r7
  from/to mapr-drill-thirdparty (
  http://repository.mapr.com/nexus/content/repositories/drill/): hostname
  in certificate didn't match: n136.network-auth.com != *.meraki.com
 OR
  *.meraki.com OR meraki.com - [Help 1]
 
 
  Daniel
  --
  Daniel Barclay
  MapR Technologies
 

[jira] [Created] (DRILL-3064) TestSimpleExternalSort is completely @Ignored

[Chris Westin (JIRA)]

Chris Westin created DRILL-3064:
---

 Summary: TestSimpleExternalSort is completely @Ignored
 Key: DRILL-3064
 URL: https://issues.apache.org/jira/browse/DRILL-3064
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Relational Operators
Affects Versions: 0.9.0
Reporter: Chris Westin
Assignee: Chris Westin


There is an @Ignore at the top of the class. We should either fix this test or 
get rid of it so we don't waste time launching it during regression testing.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34173/#review83673
---


Ship it (non-binding)

- Chris Westin


On May 13, 2015, 3:05 p.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34173/
 ---
 
 (Updated May 13, 2015, 3:05 p.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-3053
 https://issues.apache.org/jira/browse/DRILL-3053
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 added an unchecked injection site in ChildAllocator.buffer(int, int). It's 
 only enabled when assertions are enabled.
 You can throw any unchecked or Error exception, but if you throw a 
 NullPointerException the buffer() method will just return null without 
 throwing any exception to simulate a normal allocation failure
 
 
 Diffs
 -
 
   
 common/src/main/java/org/apache/drill/common/exceptions/DrillRuntimeException.java
  abc7065 
   common/src/main/java/org/apache/drill/common/exceptions/UserException.java 
 a67cb3f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
  9670c7e 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/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
 ---
 
 testing...
 
 
 Thanks,
 
 abdelhakim deneche
 

Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so far in DRILL-1942 to 1.0.0

[Chris Westin]

 On May 12, 2015, 9:13 a.m., Jacques Nadeau wrote:
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java, 
  line 218
  https://reviews.apache.org/r/34075/diff/1/?file=956037#file956037line218
 
  Shouldn't this and above be target.clear() rather than reaching inside?

I go back and forth on it. As I've mentioned before, I'd like to see us move to 
not treating the empty buffer specially. One of the changes there is to have a 
close() that is different from clear(), which does a data.release(1), and then 
sets data to null. I tried that out, and found that some vectors are 
resurrected after being closed, and are used again. So I started going with the 
check for null.

I can change it to clear() if you like. What's your preference?


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/#review83413
---


On May 12, 2015, 8:22 a.m., Chris Westin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34075/
 ---
 
 (Updated May 12, 2015, 8:22 a.m.)
 
 
 Review request for drill and Jacques Nadeau.
 
 
 Bugs: DRILL-3033
 https://issues.apache.org/jira/browse/DRILL-3033
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 Fixes some missing buffer retains() and missing vector clears().
 
 
 Diffs
 -
 
   exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810 
   exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb 
   exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 
 659d99b 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
  8e2ce96 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java
  c233ac5 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
  369c0ec 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java
  c3e70f5 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java
  0430f1b 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java
  1187bd6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
  5d990f0 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
  ca6d83c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java
  8748aaf 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
  e559ece 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
  3159811 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java
  9b97e1c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
  f7786b7 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 
 308a8bc 
   exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java 
 ae5fad5 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
  78846dc 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java
  b615b66 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java
  2b1dff0 
 
 Diff: https://reviews.apache.org/r/34075/diff/
 
 
 Testing
 ---
 
 mvn install
 precommit suite
 
 
 Thanks,
 
 Chris Westin
 

Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so far in DRILL-1942 to 1.0.0

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/
---

(Updated May 12, 2015, 11:57 a.m.)


Review request for drill and Jacques Nadeau.


Changes
---

Addressed review request.


Bugs: DRILL-3033
https://issues.apache.org/jira/browse/DRILL-3033


Repository: drill-git


Description
---

Fixes some missing buffer retains() and missing vector clears().


Diffs (updated)
-

  exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810 
  exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb 
  exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b 
  
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
 8e2ce96 
  
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java 
c233ac5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
 369c0ec 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java
 c3e70f5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java
 0430f1b 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java
 1187bd6 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
 5d990f0 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
 ca6d83c 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java
 8748aaf 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
 e559ece 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
 3159811 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java
 9b97e1c 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
 f7786b7 
  exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 
308a8bc 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java 
ae5fad5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
 78846dc 
  
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java
 b615b66 
  
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java
 2b1dff0 

Diff: https://reviews.apache.org/r/34075/diff/


Testing
---

mvn install
precommit suite


Thanks,

Chris Westin

[jira] [Created] (DRILL-3044) Very deep record batch fetching stack for single table query (TestTpchLimit0.tpch01)

[Chris Westin (JIRA)]

Chris Westin created DRILL-3044:
---

 Summary: Very deep record batch fetching stack for single table 
query (TestTpchLimit0.tpch01)
 Key: DRILL-3044
 URL: https://issues.apache.org/jira/browse/DRILL-3044
 Project: Apache Drill
  Issue Type: Bug
  Components: Query Planning  Optimization
Affects Versions: 0.9.0
Reporter: Chris Westin
Assignee: Jinfeng Ni


I ran TestTpchLimit0 in a constrained memory environment while hunting for a 
memory leak.

Here are the startup parameters (from Eclipse's test launch configuration):
-Xms512m
-Xmx3g
-Ddrill.exec.http.enabled=false
-Ddrill.exec.sys.store.provider.local.write=false
-Dorg.apache.drill.exec.server.Drillbit.system_options=org.apache.drill.exec.compile.ClassTransformer.scalar_replacement=on
-XX:MaxPermSize=256M -XX:MaxDirectMemorySize=3072M
-XX:+CMSClassUnloadingEnabled -ea
-Ddrill.exec.memory.top.max=67108864

Except for the last value, these were taken from the root pom.xml; the last 
value constrains the amount of direct memory used to 64M. (We're looking for 
leaks that happen when queries fail to allocate memory and have to be cancelled 
and aren't cleaned up properly).

I find that there is indeed a leak for tpch01 when the fragment is cleaned up. 
tpch01 looks like this:

select
  l_returnflag,
  l_linestatus,
  sum(l_quantity) as sum_qty,
  sum(l_extendedprice) as sum_base_price,
  sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
  sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
  avg(l_quantity) as avg_qty,
  avg(l_extendedprice) as avg_price,
  avg(l_discount) as avg_disc,
  count(*) as count_order
from
  cp.`tpch/lineitem.parquet`
where
  l_shipdate = date '1998-12-01' - interval '120' day (3)
group by
  l_returnflag,
  l_linestatus

order by
  l_returnflag,
  l_linestatus;

Basically a single table query with a group and sort.
But in the trace file, this is the stack at the time of the creation of the 
leaked allocator:
org.apache.drill.exec.ops.FragmentContext.getNewChildAllocator:302
org.apache.drill.exec.ops.OperatorContextImpl.init:43
org.apache.drill.exec.ops.FragmentContext.newOperatorContext:366
org.apache.drill.exec.store.parquet.ParquetScanBatchCreator.getBatch:70
org.apache.drill.exec.store.parquet.ParquetScanBatchCreator.getBatch:1
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:140
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121
org.apache.drill.exec.physical.impl.ImplCreator.getChildren:163
org.apache.drill.exec.physical.impl.ImplCreator.getRecordBatch:121

Re: Review Request 34030: DRILL-3022: ensure sequential shutdown of drillbits

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34030/#review83206
---



exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
https://reviews.apache.org/r/34030/#comment134108

Sure, I'll keep my version, and it'll go in with my current work.


- Chris Westin


On May 10, 2015, 11:37 p.m., Hanifi Gunes wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34030/
 ---
 
 (Updated May 10, 2015, 11:37 p.m.)
 
 
 Review request for drill, Chris Westin and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 DRILL-3022: ensure sequential shutdown of drillbits
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 
 531253e4245a1a4f50216359ed5591f0f1804b31 
 
 Diff: https://reviews.apache.org/r/34030/diff/
 
 
 Testing
 ---
 
 unit
 
 
 Thanks,
 
 Hanifi Gunes
 

Re: Review Request 34030: DRILL-3022: ensure sequential shutdown of drillbits

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34030/#review83203
---


With the addition of a comment explaining what's going on here, ship it 
(non-binding).


exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java
https://reviews.apache.org/r/34030/#comment134106

I also added this block comment before the synchronized block in my version:

/*
 * We can avoid metrics deregistration concurrency issues by only 
closing
 * one drillbit at a time. To enforce that, we synchronize on a 
convenient
 * singleton object.
 */


- Chris Westin


On May 10, 2015, 11:37 p.m., Hanifi Gunes wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34030/
 ---
 
 (Updated May 10, 2015, 11:37 p.m.)
 
 
 Review request for drill, Chris Westin and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 DRILL-3022: ensure sequential shutdown of drillbits
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 
 531253e4245a1a4f50216359ed5591f0f1804b31 
 
 Diff: https://reviews.apache.org/r/34030/diff/
 
 
 Testing
 ---
 
 unit
 
 
 Thanks,
 
 Hanifi Gunes
 

Re: Review Request 34074: DRILL-3035 Create ControlsInjector interface to enforce implementing methods

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34074/#review83357
---


Ship it (non-binding)

- Chris Westin


On May 11, 2015, 10:35 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34074/
 ---
 
 (Updated May 11, 2015, 10:35 p.m.)
 
 
 Review request for drill, Chris Westin and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 [DRILL-3035](https://issues.apache.org/jira/browse/DRILL-3035): Created 
 ControlsInjector interface to enforce method implementations
 + Fixes potential NPEs from site classes that use injectInterruptiblePause
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
  6176f77 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
  76dc91c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
  5d990f0 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerDecorator.java
  c355070 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java
  e40fe54 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
  6961ee6 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java
  a893da1 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ControlsInjector.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java
  387d300 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
  bb13d1f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 bf62ccb 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  d96e6d6 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  c98f54c 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestExceptionInjection.java
  e3558a1 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  ba29c58 
 
 Diff: https://reviews.apache.org/r/34074/diff/
 
 
 Testing
 ---
 
 Passes regression and unit tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

Re: Review Request 34075: DRILL-3033: Add memory leak fixes found so far in DRILL-1942 to 1.0.0

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34075/
---

(Updated May 11, 2015, 10:58 p.m.)


Review request for drill and Jacques Nadeau.


Bugs: DRILL-3033
https://issues.apache.org/jira/browse/DRILL-3033


Repository: drill-git


Description
---

Fixes some missing buffer retains() and missing vector clears().


Diffs
-

  exec/java-exec/src/main/codegen/templates/FixedValueVectors.java 7d85810 
  exec/java-exec/src/main/codegen/templates/NullableValueVectors.java 9d03efb 
  exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java 659d99b 
  
exec/java-exec/src/main/java/org/apache/drill/exec/cache/VectorAccessibleSerializable.java
 8e2ce96 
  
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java 
c233ac5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
 369c0ec 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/TopNBatch.java
 c3e70f5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java
 0430f1b 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatchBuilder.java
 1187bd6 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java
 5d990f0 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
 ca6d83c 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java
 8748aaf 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortRecordBatchBuilder.java
 e559ece 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
 3159811 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/MSortTemplate.java
 9b97e1c 
  
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/PriorityQueueCopierTemplate.java
 f7786b7 
  exec/java-exec/src/main/java/org/apache/drill/exec/record/WritableBatch.java 
308a8bc 
  exec/java-exec/src/main/java/org/apache/drill/exec/vector/BitVector.java 
ae5fad5 
  
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/AbstractMapVector.java
 78846dc 
  
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java
 b615b66 
  
exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestValueVector.java
 2b1dff0 

Diff: https://reviews.apache.org/r/34075/diff/


Testing (updated)
---

mvn install
precommit suite - has one failure, will try again to see what happens because:
Execution Failures:
/root/private-sql-hadoop-test/framework/resources/Precommit/Functional/complex/parquet/complex274.q
Query: 
null
Failed with exception
java.sql.SQLException: SYSTEM ERROR: 
org.apache.drill.common.exceptions.ExecutionSetupException: Two processes tried 
to change a plugin at the same time.


[Error Id: 78607318-d1fb-4d8d-bb60-c79d6d1f978c on drillats3.qa.lab:31010]
at org.apache.drill.jdbc.DrillCursor.next(DrillCursor.java:161)
at 
org.apache.drill.jdbc.impl.DrillResultSetImpl.execute(DrillResultSetImpl.java:167)
at 
org.apache.drill.jdbc.impl.DrillResultSetImpl.execute(DrillResultSetImpl.java:56)
at 
net.hydromatic.avatica.AvaticaConnection.executeQueryInternal(AvaticaConnection.java:404)
at 
net.hydromatic.avatica.AvaticaStatement.executeQueryInternal(AvaticaStatement.java:351)
at 
net.hydromatic.avatica.AvaticaStatement.executeQuery(AvaticaStatement.java:78)
at 
org.apache.drill.jdbc.impl.DrillStatementImpl.executeQuery(DrillStatementImpl.java:89)
at 
org.apache.drill.test.framework.DrillTestJdbc.executeSetupQuery(DrillTestJdbc.java:127)


Thanks,

Chris Westin

Re: Review Request 33770: DRILL-2697 Pause injections should pause indefinitely until signalled

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83159
---


Ship it (non-binding)

- Chris Westin


On May 9, 2015, 10:48 a.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33770/
 ---
 
 (Updated May 9, 2015, 10:48 a.m.)
 
 
 Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites 
 wait indefinitely for a resume signal
 DrillClient sends a resume signal to UserServer. UserServer triggers a resume 
 call in the correct Foreman. Foreman resumes all pauses related to the query 
 through the Control layer.
 
 + Better error messages and more tests in TestDrillbitResilience and 
 TestPauseInjection
 + Added execution controls to operator context
 + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to 
 ControlMessageHandler
 + Added CountDownLatchInjection, useful in cases like ParititionedSender that 
 spawns multiple threads
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 
 136d8c7 
   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 
 7cc52ba 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java
  6dbd880 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
  5b4d7bd 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java
  f92bb49 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java
  a4f9fdf 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java
  ae728d8 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 
 b3b7ae9 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java
  cf98b83 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
  1171bf8 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java
  4b1cd0c 
   exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 
 96fed3a 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
  80d9790 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
  e5f9c9c 
   exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 
 a3ceb8f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java
  b6c6852 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
  c5d78cc 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 b7ef584 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  34fa639 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  ddb828c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java
  0ba91b4 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
  f526fbe 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
  b1c3fe0 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 
 8854ef3 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
  da69e9e 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  PRE-CREATION 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  5fa2b3f 
   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976 
   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c3ff58b 
   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 
 4d03073 
   protocol/src/main/protobuf/BitControl.proto 93bc33c 
   protocol/src/main/protobuf/User.proto 185a646 
 
 Diff: https://reviews.apache.org/r/33770/diff/
 
 
 Testing
 ---
 
 Passes all unit tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

Re: Review Request 33903: DRILL-2878: FragmentExecutor.closeOutResources() is not called if an exception happens in the Foreman before the fragment executor starts running

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/#review83005
---


One improvement for the unit test, but otherwise LGTM (non-binding).


exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java
https://reviews.apache.org/r/33903/#comment133883

To make sure you get the expected exception here, declare a boolean 
variable initialized to false before the try block. Then set it to true inside 
the catch block. After the catch block use assertTrue() on it.


- Chris Westin


On May 8, 2015, 9:12 a.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33903/
 ---
 
 (Updated May 8, 2015, 9:12 a.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-2878
 https://issues.apache.org/jira/browse/DRILL-2878
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 *INITIAL-PATCH*
 
 This is a quick fix that seem to solve the problem, at least for the cases 
 I am able to reproduce it for. closeOutResources() shouldn't throw any 
 exception at this point because we didn't even start running, and any 
 allocation failures will be suppressed (do we want this ?)
 
 If this fix is acceptable I will go ahead and add a private volatile boolean 
 startedRunning that will be set to true in run() and used in cancel() to 
 check if we need to call closeOutResources().
 
 I will also add a unit test, I know how to reproduce the problem for both the 
 root and intermediate fragments, but I still need to find a proper way to 
 detect that those fragments were not closed properly.
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  ddb828c 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/work/fragment/TestFragmentExecutorCancel.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/33903/diff/
 
 
 Testing
 ---
 
 all unit tests are passing along with functional/tpch100
 
 I will redo the tests once DRILL-2757 has been committed
 
 
 Thanks,
 
 abdelhakim deneche
 

[jira] [Created] (DRILL-2996) ValueVectors shouldn't call reAlloc() in a while() loop

[Chris Westin (JIRA)]

Chris Westin created DRILL-2996:
---

 Summary: ValueVectors shouldn't call reAlloc() in a while() loop
 Key: DRILL-2996
 URL: https://issues.apache.org/jira/browse/DRILL-2996
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Data Types
Reporter: Chris Westin
Assignee: Daniel Barclay (Drill)


Instead, reAlloc() should be change to take a new minimum size as an 
argument. This value is just the value used to determine the while loops' 
termination. Then reAlloc() can figure out how much more to allocate once and 
for all, instead of possibly reallocating and copying more than once, and it 
can make sure that the size doesn't overflow (we've seen some instances of the 
allocator being called with negative sizes).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 33770: DRILL-2697 Pause injections should pause indefinitely until signalled

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review83124
---


Ship it (non-binding)

- Chris Westin


On May 8, 2015, 6:24 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33770/
 ---
 
 (Updated May 8, 2015, 6:24 p.m.)
 
 
 Review request for drill, Chris Westin, Jacques Nadeau, and Venki Korukanti.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 [DRILL-2697](https://issues.apache.org/jira/browse/DRILL-2697): Pauses sites 
 wait indefinitely for a resume signal
 DrillClient sends a resume signal to UserServer. UserServer triggers a resume 
 call in the correct Foreman. Foreman resumes all pauses related to the query 
 through the Control layer.
 
 + Better error messages and more tests in TestDrillbitResilience and 
 TestPauseInjection
 + Added execution controls to operator context
 + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to 
 ControlMessageHandler
 + Added CountDownLatchInjection, useful in cases like ParititionedSender that 
 spawns multiple threads
 
 
 Diffs
 -
 
   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 
 9924704 
   exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContext.java 
 7cc52ba 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java
  6dbd880 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
  5b4d7bd 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlRpcConfig.java
  37730e3 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/control/ControlTunnel.java
  a4f9fdf 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserRpcConfig.java
  88592d4 
   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java 
 9e929de 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/store/pojo/PojoRecordReader.java
  cf98b83 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjection.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java
  PRE-CREATION 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
  1171bf8 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java
  4b1cd0c 
   exec/java-exec/src/main/java/org/apache/drill/exec/testing/Injection.java 
 96fed3a 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/NoOpControlsInjector.java
  80d9790 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
  e5f9c9c 
   exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java 
 a3ceb8f 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlHandlerImpl.java
  b6c6852 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/ControlMessageHandler.java
  c5d78cc 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 
 49d0c94 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
  34fa639 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  ddb828c 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java
  0ba91b4 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/NonRootFragmentManager.java
  f526fbe 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/RootFragmentManager.java
  b1c3fe0 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java 
 8854ef3 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
  da69e9e 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestCountDownLatchInjection.java
  PRE-CREATION 
   
 exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
  5fa2b3f 
   protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 470e976 
   protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java c072a47 
   protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java 
 4d03073 
   protocol/src/main/protobuf/BitControl.proto 93bc33c 
   protocol/src/main/protobuf/User.proto 59e22ae 
 
 Diff: https://reviews.apache.org/r/33770/diff/
 
 
 Testing
 ---
 
 Passes all unit tests.
 
 
 Thanks,
 
 Sudheesh Katkam
 

Re: [GitHub] drill pull request: DRILL-2961: Throw SQLException when attempting...

[Chris Westin]

The github apache account is a read-only mirror of http://git.apache.org/ /
https://git-wip-us.apache.org/repos/asf?p=drill.git
(see http://www.apache.org/dev/git.html). So unfortunately, we don't get to
use the nice github pull mechanics.

Instead, you need to submit a patch for review on the Apache review board
at reviews.apache.org. Then it can be reviewed and a committer can merge it
if it passes muster. The whole process is described here:
http://drill.apache.org/docs/apache-drill-contribution-guidelines/ .


On Wed, May 6, 2015 at 9:39 AM, kkhatua g...@git.apache.org wrote:

 GitHub user kkhatua opened a pull request:

 https://github.com/apache/drill/pull/71

 DRILL-2961: Throw SQLException when attempting to set query timeout

 Statement.setQueryTimeout(int seconds) is a No-Op, leading the user to
 believe that a timeout has been set. This provides the correct response
 (SQLException for 'method not supported')

 You can merge this pull request into a Git repository by running:

 $ git pull https://github.com/kkhatua/drill master

 Alternatively you can review and apply these changes as the patch at:

 https://github.com/apache/drill/pull/71.patch

 To close this pull request, make a commit to your master/trunk branch
 with (at least) the following in the commit message:

 This closes #71

 
 commit 1d3ffe84f3237d614b524702e89be8cce1509931
 Author: Kunal Khatua kkha...@maprtech.com
 Date:   2015-05-06T16:37:56Z

 DRILL-2961: Throw SQLException when attempting to set query timeout

 Statement.setQueryTimeout(int seconds) is a No-Op, leading the user to
 believe that a timeout has been set. This provides the correct response
 (SQLException for 'method not supported')

 


 ---
 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 is enabled but not working, please
 contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
 with INFRA.
 ---

Re: Review Request 33903: DRILL-2878: FragmentExecutor.closeOutResources() is not called if an exception happens in the Foreman before the fragment executor starts running

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33903/#review82729
---


Ship it (non-binding)

Why do you need startedRunning if a null root is meant to detect the same thing?

- Chris Westin


On May 6, 2015, 11:46 a.m., abdelhakim deneche wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33903/
 ---
 
 (Updated May 6, 2015, 11:46 a.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Bugs: DRILL-2878
 https://issues.apache.org/jira/browse/DRILL-2878
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 *INITIAL-PATCH*
 
 This is a quick fix that seem to solve the problem, at least for the cases 
 I am able to reproduce it for. closeOutResources() shouldn't throw any 
 exception at this point because we didn't even start running, and any 
 allocation failures will be suppressed (do we want this ?)
 
 If this fix is acceptable I will go ahead and add a private volatile boolean 
 startedRunning that will be set to true in run() and used in cancel() to 
 check if we need to call closeOutResources().
 
 I will also add a unit test, I know how to reproduce the problem for both the 
 root and intermediate fragments, but I still need to find a proper way to 
 detect that those fragments were not closed properly.
 
 
 Diffs
 -
 
   
 exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
  ddb828c 
 
 Diff: https://reviews.apache.org/r/33903/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 abdelhakim deneche
 

Re: TODOs in comments

[Chris Westin]

I like that idea, but how would the build know what JIRA you're working on?

On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse altekruseja...@gmail.com
wrote:

 We should also consider adding something to the build that will automate
 the process of checking for todo comments containing the JIRA number being
 fixed. This would allow reviewers to easily verify that a JIRA being closed
 is not leaving related TODOs in the code (possibly unit tests added by the
 reporter of the issue, or comments mentioned in another patch that wanted
 to relate a problem they saw in a known outstanding JIRA). This can be
 mitigated by mentioning the relevant areas in the code when filing a JIRA,
 but this would also be a helpful safety net to keep the code cleaner.

 - Jason

 On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam skat...@maprtech.com
 wrote:

  Hey y’all,
 
  For consistency across code, Chris had recommended using TODO(DRILL-)
  format for todos in comments, where  is the JIRA number. If there are
  no objections, let’s try to stick to that format.
 
  Thank you,
  Sudheesh

Re: Review Request 33770: DRILL-2697 Pause injections should pause indefinitely until signalled

[Chris Westin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33770/#review82573
---


A few minor things, along with some questions about resumeAll and the 
implications of that. I may have more suggestions based on the answers to my 
questions. Some of those might be best answered in the form of comments in the 
code or class javadoc because it seems likely others will have the same 
questions later.


exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java
https://reviews.apache.org/r/33770/#comment133305

I'm not sure about this waiting uninterruptibly. We're going to start using 
Thread.interrupt() to regain control of threads that are blocked waiting on 
queues or sockets if the fragment they are running on behalf of is cancelled. 
Seems like this should be handled in the same way. I can talk to you about that 
more, and Venki can tell you about what he's doing in this area.



exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
https://reviews.apache.org/r/33770/#comment133310

Should this wait on acceptExternalEvents, in the same way cancel() does? 
What happens if we're not completely set up and we execute these? The 
queryContext one seems like it might be ok, but the queryManager one definitely 
seems like it could be a problem if this comes in before all the remote 
fragments are set up.



exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
https://reviews.apache.org/r/33770/#comment133307

All of them? I thought we were going to control this by queryId?



exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
https://reviews.apache.org/r/33770/#comment133309

Should you clear the resume flag after this?



exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
https://reviews.apache.org/r/33770/#comment133316

Does this mean that I can't have multiple pauses in the execution thread 
that will all work for a single query? For example, suppose I inject two pauses 
at different phases of execution: one just after planning and remote fragments 
are set up, and another after the first batch of results are returned. Will 
they both work, or will only the first one work?



exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentManager.java
https://reviews.apache.org/r/33770/#comment133319

How about some javadoc explaining what this means? From this it's not at 
all obvious that this is about injected pauses instead of something like 
suspend/resume.

Would unpause() be a better name?



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
https://reviews.apache.org/r/33770/#comment133322

I couldn't see why you need to use a Pointer here instead of just the 
Exception.



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
https://reviews.apache.org/r/33770/#comment133323

Instead of TC 1, TC 2, etc, can you please just copy-paste the one-line 
descriptions of these test cases from the original. Much easier if it's all 
described here inline.



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
https://reviews.apache.org/r/33770/#comment133324

Why not use assertTrue() here?



exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
https://reviews.apache.org/r/33770/#comment133325

assertTrue()?



exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java
https://reviews.apache.org/r/33770/#comment133327

break the line at .setUserName() so only one attribute is set per line.



protocol/src/main/protobuf/BitControl.proto
https://reviews.apache.org/r/33770/#comment133329

Can we clean up this whitespace, and leave just a newline?


- Chris Westin


On May 1, 2015, 6:09 p.m., Sudheesh Katkam wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33770/
 ---
 
 (Updated May 1, 2015, 6:09 p.m.)
 
 
 Review request for drill, Chris Westin and Jacques Nadeau.
 
 
 Repository: drill-git
 
 
 Description
 ---
 
 DRILL-2697: Pauses sites wait indefinitely for a resume signal
 DrillClient sends a resume signal to UserServer. UserServer triggers a resume 
 call in the correct Foreman. Foreman resumes all pauses related to the query 
 through the Control layer.
 
 + Fix for bug in FragmentExecutor#closeOutResources
 + Better error messages and more tests in TestDrillbitResilience and 
 TestPauseInjection
 + Added execution controls to operator context
 + Removed ControlMessageHandler interface, renamed ControlHandlerImpl to 
 ControlMessageHandler

[jira] [Created] (DRILL-2923) Ensure all unit tests pass without assertions enabled

[Chris Westin (JIRA)]

Chris Westin created DRILL-2923:
---

 Summary: Ensure all unit tests pass without assertions enabled
 Key: DRILL-2923
 URL: https://issues.apache.org/jira/browse/DRILL-2923
 Project: Apache Drill
  Issue Type: Bug
  Components: Execution - Flow
Reporter: Chris Westin
Assignee: Chris Westin


I recently discovered that the test TestValueVector.java only passes if 
assertions are enabled (-ea to the JVM). This means that there are conditions 
which will fail in the wild, because production users don't run with assertions 
enabled. Someone needs to do a test run of mvn install with the -ea removed 
from the surefire command line and make sure that everything passes, and if 
not, we should fix them. We should also find a way to do this periodically, as 
part of CI, or as part of our regular test suites.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)