Unknown clang-tidy failure in GVO

[Sailesh Mukil]

Does anyone know the cause for this failure in the clang-tidy run in GVO?
It's something to do with hadoop-lzo.

https://jenkins.impala.io/job/clang-tidy-ub1604/78/consoleFull


*15:52:44*   [javadoc] Generating
/home/ubuntu/hadoop-lzo/build/docs/api/help-doc.html...*15:52:44*
[javadoc] 1 error*15:52:44*   [javadoc] 1 warning*15:52:44*
[javadoc] javadoc: error - Error while reading file
/home/ubuntu/hadoop-lzo/src/java/overview.html*15:52:44* *15:52:44*
package:*15:52:44* [mkdir] Created dir:
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15*15:52:44* [mkdir]
Created dir: /home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib*15:52:44*
[mkdir] Created dir:
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/docs*15:52:44*
[mkdir] Created dir: /home/ubuntu/hadoop-lzo/lib*15:52:44*  [copy]
Copying 56 files to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib*15:52:44*
[copy] Copying 1 file to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15*15:52:44*  [exec]
Created 
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/docs*15:52:44*
 [exec] Copying libraries in /docs to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/docs/*15:52:44*
 [exec] Created
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/hadoop-lzo-0.4.15.jar*15:52:44*
 [exec] Copying libraries in /hadoop-lzo-0.4.15.jar to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/hadoop-lzo-0.4.15.jar/*15:52:44*
 [exec] Created
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/lib*15:52:44*
 [exec] Copying libraries in /lib to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/lib/*15:52:44*
 [exec] Created
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/Linux-amd64-64*15:52:44*
 [exec] Copying libraries in
/home/ubuntu/hadoop-lzo/build/native/Linux-amd64-64/lib to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/lib/native/Linux-amd64-64/*15:52:44*
 [exec] /home/ubuntu/hadoop-lzo/src/native/packageNativeHadoop.sh:
44: cd: can't cd to /docs/*15:52:44*  [exec] tar:
*gplcompression*: Cannot stat: No such file or directory*15:52:44*
 [exec] tar: Exiting with failure status due to previous
errors*15:52:44*  [exec]
/home/ubuntu/hadoop-lzo/src/native/packageNativeHadoop.sh: 44: cd:
can't cd to /hadoop-lzo-0.4.15.jar/*15:52:44*  [exec] tar:
*gplcompression*: Cannot stat: No such file or directory*15:52:44*
 [exec] tar: Exiting with failure status due to previous
errors*15:52:44*  [exec] tar: *gplcompression*: Cannot stat: No
such file or directory*15:52:44*  [exec] tar: Exiting with failure
status due to previous errors*15:52:44*  [copy] Copying 77 files
to /home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/docs*15:52:44*
[copy] Copying 1 file to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15*15:52:44*  [copy]
Copying 4 files to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/ivy*15:52:44*
[copy] Copying 1 file to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15*15:52:44*  [copy]
Copying 64 files to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15/src*15:52:44*
[copy] Copying 1 file to
/home/ubuntu/hadoop-lzo/build/hadoop-lzo-0.4.15

Re: [VOTE] Graduate to a TLP

[Sailesh Mukil]

+1

On Tue, Oct 17, 2017 at 9:41 PM, Marcel Kornacker  wrote:

> +1
>
> On Tue, Oct 17, 2017 at 9:30 PM, Jeszy  wrote:
> > +1
> >
> > On 18 October 2017 at 03:40, Tim Armstrong 
> wrote:
> >> +1
> >>
> >> On 17 Oct. 2017 8:38 pm, "Alexander Behm" 
> wrote:
> >>
> >>> +1
> >>>
> >>> On Tue, Oct 17, 2017 at 8:18 PM, Taras Bobrovytsky <
> taras...@apache.org>
> >>> wrote:
> >>>
> >>> > +1
> >>> >
> >>> > On Tue, Oct 17, 2017 at 7:56 PM, Michael Ho 
> wrote:
> >>> >
> >>> > > +1
> >>> > >
> >>> > > On Tue, Oct 17, 2017 at 7:25 PM, Thomas Tauber-Marshall <
> >>> > > tmarsh...@cloudera.com> wrote:
> >>> > >
> >>> > > > +1
> >>> > > >
> >>> > > > On Tue, Oct 17, 2017 at 9:12 PM Bharath Vissapragada <
> >>> > > > bhara...@cloudera.com>
> >>> > > > wrote:
> >>> > > >
> >>> > > > > +1
> >>> > > > >
> >>> > > > > On Tue, Oct 17, 2017 at 7:10 PM, Mostafa Mokhtar <
> >>> > > mmokh...@cloudera.com>
> >>> > > > > wrote:
> >>> > > > >
> >>> > > > > > +1
> >>> > > > > >
> >>> > > > > > Thanks
> >>> > > > > > Mostafa
> >>> > > > > >
> >>> > > > > > > On Oct 17, 2017, at 7:09 PM, Brock Noland  >
> >>> > wrote:
> >>> > > > > > >
> >>> > > > > > > +1
> >>> > > > > > >
> >>> > > > > > >> On Tue, Oct 17, 2017 at 9:07 PM, Lars Volker <
> l...@cloudera.com
> >>> >
> >>> > > > wrote:
> >>> > > > > > >> +1
> >>> > > > > > >>
> >>> > > > > > >>> On Oct 17, 2017 19:07, "Jim Apple"  >
> >>> > wrote:
> >>> > > > > > >>>
> >>> > > > > > >>> Following our discussion
> >>> > > > > > >>> https://lists.apache.org/thread.html/
> >>> > > > 2f5db4788aff9b0557354b9106c032
> >>> > > > > > >>> 8a29c1f90c1a74a228163949d2@%3Cdev.impala.apache.org%3E
> >>> > > > > > >>> , I propose that we graduate to a TLP. According to
> >>> > > > > > >>> https://incubator.apache.org/guides/graduation.html#
> >>> > > > > > >>> community_graduation_vote
> >>> > > > > > >>> this is not required, and https://impala.apache.org/
> >>> > bylaws.html
> >>> > > > does
> >>> > > > > > not
> >>> > > > > > >>> say whose votes are "binding" in a graduation vote, so
> all
> >>> > > > community
> >>> > > > > > >>> members are welcome to vote.
> >>> > > > > > >>>
> >>> > > > > > >>> This will remain open 72 hours. I will be notifying
> >>> > > > general@incubator
> >>> > > > > > it
> >>> > > > > > >>> is
> >>> > > > > > >>> occurring.
> >>> > > > > > >>>
> >>> > > > > > >>> This is my +1.
> >>> > > > > > >>>
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> > >
> >>> > >
> >>> > > --
> >>> > > Thanks,
> >>> > > Michael
> >>> > >
> >>> >
> >>>
>

Re: NPE happened while catalogd running

[Sailesh Mukil]

Hi,

Apologies for the late response. We had debugged and tracked down and filed
a bug in both the Java security library and the kerberos thirdparty code
here:
http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8186576
http://mailman.mit.edu/pipermail/krbdev/2017-August/012809.html

A workaround for now is to increase your ticket lifetime to about a day and
the renew lifetime to 7 days (which is the default). This bug is less
likely to be hit with larger ticket lifetimes.

On Wed, Sep 20, 2017 at 7:59 PM, yu feng  wrote:

> In our env. we encountered the problem like IMPALA-2655
> , and It
> happened periodically， Did anyone meet the problem and solve it, or some
> suggestions. Thanks a lot.
>

Re: expr-test stuck in getJNIEnv

[Sailesh Mukil]

The upstream bug is posted below. I've requested a backport to the HDFS
version Impala depends on, but it hasn't happened yet. I'll make another
push for it.

https://issues.apache.org/jira/browse/HDFS-11851

On Sun, Sep 10, 2017 at 4:01 PM, Jim Apple  wrote:

> It was . bin/set-classpath.sh. Forgot about that one. Thanks, Henry!
>
> On Sun, Sep 10, 2017 at 3:31 PM, Henry Robinson  wrote:
> > I've seen this deadlock before although not in expr-test. I can't
> remember
> > exactly how I cleared it but I believe it was either:
> >
> > 1. make fe && . bin/set-classpath.sh
> > 2. bin/create-test-configuration.sh
> >
> > Sailesh knows about the upstream HDFS bug which I think has been fixed
> but
> > not incorporated into Impala's dependencies.
> >
> > On Sun, Sep 10, 2017 at 1:42 PM Jim Apple  wrote:
> >
> >> When I run expr-test, it gets stuck in getJNIEnv(). Here's the full
> stack
> >> trace:
> >>
> >> #0  __lll_lock_wait () at
> >> ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> >> #1  0x74885dbd in __GI___pthread_mutex_lock (mutex=0x45fe5a0
> >> ) at ../nptl/pthread_mutex_lock.c:80
> >> #2  0x02cf79f6 in mutexLock (m=) at
> >>
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/os/posix/mutexes.c:28
> >> #3  0x02cf01b7 in setTLSExceptionStrings (rootCause=0x0,
> >> stackTrace=0x0) at
> >>
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/jni_helper.c:581
> >> #4  0x02cf7f77 in printExceptionAndFreeV (env=0x4f221e8,
> >> exc=0x4eb6a00, noPrintFlags=, fmt=0x33d7994
> >> "loadFileSystems", ap=0x7fff9da0)
> >> at
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/exception.c:183
> >> #5  0x02cf81dd in printExceptionAndFree (env=,
> >> exc=, noPrintFlags=, fmt= >> out>)
> >> at
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/exception.c:213
> >> #6  0x02cf0faf in getGlobalJNIEnv () at
> >>
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/jni_helper.c:463
> >> #7  getJNIEnv () at
> >>
> >> /data/2/jenkins/workspace/impala-hadoop-dependency/
> hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/native/
> libhdfs/jni_helper.c:528
> >> #8  0x01a0116a in impala::JniUtil::Init () at
> >> be/src/util/jni-util.cc:105
> >> #9  0x014fd881 in impala::InitCommonRuntime (argc=1,
> >> argv=0x7fffa628, init_jvm=true,
> >> test_mode=impala::TestInfo::BE_TEST) at be/src/common/init.cc:236
> >> #10 0x0143da3f in main (argc=1, argv=0x7fffa628) at
> >> be/src/exprs/expr-test.cc:7420
> >>
> >> I've tried git fetch, bin/clean.sh, running with the minicluster on,
> >> running with the minicluster off, running with the impala cluster on,
> >> running with it off, running in release mode, debug mode, in gdb, and
> >> out of gdb.
> >>
> >> Has anyone else seen this and escaped from its clutches?
> >>
>

CVE-2017-5640 Apache Impala (incubating) Information Disclosure

[Sailesh Mukil]

CVE-2017-5640 Apache Impala (incubating) Information Disclosure

Severity: High

Versions Affected:
Apache Impala (incubating) 2.7.0 to 2.8.0

Description:
It was noticed that a malicious process impersonating an Impala daemon
could cause Impala daemons to skip authentication checks when Kerberos
is enabled (but TLS is not). If the malicious server responds with
‘COMPLETE’ before the SASL handshake has completed, the client will
consider the handshake as completed even though no exchange of
credentials has happened.

Mitigation:
Users of the affected versions should apply the following mitigation:
Upgrade to Apache Impala (incubating) 2.9.0

Credit:
This issue was identified by the Cloudera Security team.

References:
https://issues.apache.org/jira/browse/IMPALA-5005

[SECURITY] CVE-2017-5652 Apache Impala (incubating) Information Disclosure

[Sailesh Mukil]

CVE-2017-5652 Apache Impala (incubating) Information Disclosure


Severity: High


Versions Affected:

Apache Impala (incubating) 2.7.0 to 2.8.0


Description:

During a routine security analysis, it was found that one of the ports sent
data in plaintext even when the cluster was configured to use TLS. The port
in question was used by the StatestoreSubscriber class which did not use
the appropriate secure Thrift transport when TLS was turned on. It was
therefore possible for an adversary, with access to the network, to
eavesdrop on the packets going to and coming from that port and view the
data in plaintext.


Mitigation:

Users of the affected versions should apply the following mitigation:

 - Upgrade to Apache Impala (incubating) 2.9.0


Credit:
This issue was identified and reported responsibly by the Cloudera security
team.


References:
[1] https://issues.apache.org/jira/browse/IMPALA-5253

Re: GVO machines dependencies

[Sailesh Mukil]

Glad I could help! The fix is merged now:
https://github.com/awleblang/impala-setup/commit/1fcff44887de76fdb5f86948635c073d0e615f1c

On Thu, May 25, 2017 at 10:00 AM, Laszlo Gaal <laszlo.g...@cloudera.com>
wrote:

> Thanks a lot for fixing this, Sailesh, this has been bugging me for a while
> when spinning up cloud-based workstations.
>
> On Thu, May 25, 2017 at 6:18 PM, Jim Apple <jbap...@cloudera.com> wrote:
>
> > Nice find, Sailesh!
> >
> > On Wed, May 24, 2017 at 6:39 PM, Sailesh Mukil <sail...@cloudera.com>
> > wrote:
> > > The issue was that there was a bug in the install.sh script from the
> > > impala-setup/ repo that always assumed that the repo would reside in
> > > /home/$user.
> > >
> > > The Jenkins AMIs seem to have had a very old checkout (8 months old) at
> > > /home/ubuntu/ which was being used for the GVOs. The Jenkins job would
> > make
> > > a fresh checkout in /tmp/ and run install.sh from that checkout. But
> due
> > to
> > > the above bug, the script would change its sources to the old repo in
> > > /home/ubuntu. So, the new dependencies added to the project didn't get
> > > picked up. I've submitted a fix, and will redo the GVO once it's merged
> > > into impala-setup.
> > >
> > > Thanks for your help Michael and Jim!
> > >
> > > On Wed, May 24, 2017 at 4:58 PM, Michael Brown <mi...@cloudera.com>
> > wrote:
> > >
> > >> "from-scratch" is a misnomer: only the first build to run on a given
> > node
> > >> is truly from-scratch, because running impala-setup takes effect
> > >> system-wide, and is run once, when the node is created.
> > >>
> > >> For http://jenkins.impala.io:8080/job/ubuntu-14.04-from-scratch/1371/
> ,
> > >> the
> > >> worker's build history suggests it has been up roughly 17 hours.
> > >> http://jenkins.impala.io:8080/computer/ub1404-c4.4xl-gp2%20(
> > >> i-0d76efbc8de26926c)/builds
> > >> This means your recent change hasn't taken effect on "older" workers.
> > >>
> > >> Any new workers that get created should get the change.
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> On Wed, May 24, 2017 at 4:34 PM, Sailesh Mukil <sail...@cloudera.com>
> > >> wrote:
> > >>
> > >> > Thanks Michael and Jim,
> > >> >
> > >> > It looks like adding to bin/bootstrap_build.sh makes the
> > ub14-build-only
> > >> > job work fine, however, updating the impala-setup still doesn't seem
> > to
> > >> > work. I submitted a pull request that was merged by Dimitris here:
> > >> > https://github.com/awleblang/impala-setup/commits/master
> > >> >
> > >> > Also, the impala-setup chef logs don't seem to get printed in the
> > jenkins
> > >> > console output. So I'm not sure if it's actually being run or not.
> > >> >
> > >> > On Wed, May 24, 2017 at 3:37 PM, Michael Brown <mi...@cloudera.com>
> > >> wrote:
> > >> >
> > >> > > Thanks Jim; I had forgotten bin/bootstrap_build.sh.
> > >> > >
> > >> > > On Wed, May 24, 2017 at 3:14 PM, Jim Apple <jbap...@cloudera.com>
> > >> wrote:
> > >> > >
> > >> > > > I'd recommend against #2 using the node setup to install Impala
> > >> > > > dependencies. The only reason it installs openjdk is that
> Jenkins
> > >> > > > needs Java to talk to it. All of the other Impala dependencies
> are
> > >> > > > installed in the jobs themselves.
> > >> > > >
> > >> > > > On Wed, May 24, 2017 at 3:11 PM, Michael Brown <
> > mi...@cloudera.com>
> > >> > > wrote:
> > >> > > > >> Is there a way to add a new dependency to these machines?
> > >> > > > >
> > >> > > > > Our builds use two worker labels:
> > >> > > > > 1. ubuntu14.04-c4.4xlarge-gp2
> > >> > > > > 2. ub14-build-only
> > >> > > > >
> > >> > > > > 1 uses https://github.com/awleblang/impala-setup, and I think
> > you
> > >> > > should
> > >> > > > > add libffi to there. It's something you should do anyway so
> new
> > >> users
> > >> > > get
> > >> > > > > set up with the right dependencies.
> > >> > > > >
> > >> > > > > 2 is a different story, but I see that at
> > >> > > > > http://jenkins.impala.io:8080/configure that worker is
> already
> > >> > > > installing a
> > >> > > > > JDK via apt-get. You could use a similar pattern to install
> > libffi.
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
>

Re: GVO machines dependencies

[Sailesh Mukil]

The issue was that there was a bug in the install.sh script from the
impala-setup/ repo that always assumed that the repo would reside in
/home/$user.

The Jenkins AMIs seem to have had a very old checkout (8 months old) at
/home/ubuntu/ which was being used for the GVOs. The Jenkins job would make
a fresh checkout in /tmp/ and run install.sh from that checkout. But due to
the above bug, the script would change its sources to the old repo in
/home/ubuntu. So, the new dependencies added to the project didn't get
picked up. I've submitted a fix, and will redo the GVO once it's merged
into impala-setup.

Thanks for your help Michael and Jim!

On Wed, May 24, 2017 at 4:58 PM, Michael Brown <mi...@cloudera.com> wrote:

> "from-scratch" is a misnomer: only the first build to run on a given node
> is truly from-scratch, because running impala-setup takes effect
> system-wide, and is run once, when the node is created.
>
> For http://jenkins.impala.io:8080/job/ubuntu-14.04-from-scratch/1371/ ,
> the
> worker's build history suggests it has been up roughly 17 hours.
> http://jenkins.impala.io:8080/computer/ub1404-c4.4xl-gp2%20(
> i-0d76efbc8de26926c)/builds
> This means your recent change hasn't taken effect on "older" workers.
>
> Any new workers that get created should get the change.
>
>
>
>
>
> On Wed, May 24, 2017 at 4:34 PM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
>
> > Thanks Michael and Jim,
> >
> > It looks like adding to bin/bootstrap_build.sh makes the ub14-build-only
> > job work fine, however, updating the impala-setup still doesn't seem to
> > work. I submitted a pull request that was merged by Dimitris here:
> > https://github.com/awleblang/impala-setup/commits/master
> >
> > Also, the impala-setup chef logs don't seem to get printed in the jenkins
> > console output. So I'm not sure if it's actually being run or not.
> >
> > On Wed, May 24, 2017 at 3:37 PM, Michael Brown <mi...@cloudera.com>
> wrote:
> >
> > > Thanks Jim; I had forgotten bin/bootstrap_build.sh.
> > >
> > > On Wed, May 24, 2017 at 3:14 PM, Jim Apple <jbap...@cloudera.com>
> wrote:
> > >
> > > > I'd recommend against #2 using the node setup to install Impala
> > > > dependencies. The only reason it installs openjdk is that Jenkins
> > > > needs Java to talk to it. All of the other Impala dependencies are
> > > > installed in the jobs themselves.
> > > >
> > > > On Wed, May 24, 2017 at 3:11 PM, Michael Brown <mi...@cloudera.com>
> > > wrote:
> > > > >> Is there a way to add a new dependency to these machines?
> > > > >
> > > > > Our builds use two worker labels:
> > > > > 1. ubuntu14.04-c4.4xlarge-gp2
> > > > > 2. ub14-build-only
> > > > >
> > > > > 1 uses https://github.com/awleblang/impala-setup, and I think you
> > > should
> > > > > add libffi to there. It's something you should do anyway so new
> users
> > > get
> > > > > set up with the right dependencies.
> > > > >
> > > > > 2 is a different story, but I see that at
> > > > > http://jenkins.impala.io:8080/configure that worker is already
> > > > installing a
> > > > > JDK via apt-get. You could use a similar pattern to install libffi.
> > > >
> > >
> >
>

Re: GVO machines dependencies

[Sailesh Mukil]

Thanks Michael and Jim,

It looks like adding to bin/bootstrap_build.sh makes the ub14-build-only
job work fine, however, updating the impala-setup still doesn't seem to
work. I submitted a pull request that was merged by Dimitris here:
https://github.com/awleblang/impala-setup/commits/master

Also, the impala-setup chef logs don't seem to get printed in the jenkins
console output. So I'm not sure if it's actually being run or not.

On Wed, May 24, 2017 at 3:37 PM, Michael Brown  wrote:

> Thanks Jim; I had forgotten bin/bootstrap_build.sh.
>
> On Wed, May 24, 2017 at 3:14 PM, Jim Apple  wrote:
>
> > I'd recommend against #2 using the node setup to install Impala
> > dependencies. The only reason it installs openjdk is that Jenkins
> > needs Java to talk to it. All of the other Impala dependencies are
> > installed in the jobs themselves.
> >
> > On Wed, May 24, 2017 at 3:11 PM, Michael Brown 
> wrote:
> > >> Is there a way to add a new dependency to these machines?
> > >
> > > Our builds use two worker labels:
> > > 1. ubuntu14.04-c4.4xlarge-gp2
> > > 2. ub14-build-only
> > >
> > > 1 uses https://github.com/awleblang/impala-setup, and I think you
> should
> > > add libffi to there. It's something you should do anyway so new users
> get
> > > set up with the right dependencies.
> > >
> > > 2 is a different story, but I see that at
> > > http://jenkins.impala.io:8080/configure that worker is already
> > installing a
> > > JDK via apt-get. You could use a similar pattern to install libffi.
> >
>

GVO machines dependencies

[Sailesh Mukil]

I have a question regarding dependencies on the jenkins machines. (
http://jenkins.impala.io:8080/job/ubuntu-14.04-from-scratch/)

How do the Jenkins machines used to GVO have dependencies pre-packaged in
them? For eg., openssl, etc. Are they baked into an AMI?

I have a patch (https://gerrit.cloudera.org/#/c/6910/) that requires a new
dependency for Ubuntu (libffi), and therefore won't pass GVO. This didn't
show up in all my testing since the machines I used already had this
dependency in them.

Is there a way to add a new dependency to these machines?

Re: Min/Max runtime filtering on Impala-Kudu

[Sailesh Mukil]

On Tue, Mar 28, 2017 at 4:41 PM, Todd Lipcon <t...@cloudera.com> wrote:

> On Tue, Mar 28, 2017 at 4:39 PM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
>
> > On Tue, Mar 28, 2017 at 3:33 AM, Lars Volker <l...@cloudera.com> wrote:
> >
> > > Thanks for sending this around, Sailesh.
> > >
> > > parquet-column-stats.h is somewhat tied to parquet statistics and will
> > soon
> > > need more parquet-specific fields (null_count and distinct_count). It
> > will
> > > also need extension to copy strings and handle there memory when
> tracking
> > > min/max values for variable length data across row batches. I'm not
> sure
> > > how these changes will affect its suitability for your purposes, but I
> > > thought I'd give a quick heads up.
> > >
> > > Thanks, Lars. In that case, it looks like the right path to take is to
> > implement a simpler version of this for this patch. Thanks for weighing
> in.
> >
> > On Mon, Mar 27, 2017 at 11:35 PM, Todd Lipcon <t...@cloudera.com> wrote:
> > >
> > > > Sounds reasonable to me as well.
> > > >
> > > > The only thing I'd add is that, if it's easy to design the code to be
> > > > extended to pushing small 'IN (...)' predicate for low-cardinality
> > > filters,
> > > > that would be great. eg if the filter can start as an IN(...) and
> then
> > if
> > > > it exceeds 32K (or whatever arbitrary threshold), "collapse" it to
> the
> > > > min/max range predicate?
> > > >
> > > > This should have a big advantage for partition pruning in
> > low-cardinality
> > > > joins against hash-partitioned tables.
> > > >
> > > > -Todd
> > > >
> > >
> >
> > Thanks, Todd. You're right, the IN predicate filters would be a great
> > addition. We can add that as a follow on patch, just to keep this patch
> > simple for now.
> >
>
> Sure, just wanted to suggest that whatever path's taken now, implementation
> wise, is reasonably easy to extend to the IN() case without too much
> backtracking.
>

Yes, once we refactor the filter distribution mechanism to accommodate the
KuduScanNode, extending it to support the IN() filters shouldn't be too
complicated.


> -Todd
>
>
> >
> > >
> > > >
> > > > On Mon, Mar 27, 2017 at 2:29 PM, Matthew Jacobs <m...@cloudera.com>
> > wrote:
> > > >
> > > > > Thanks for writing this up, Sailesh. It sounds reasonable.
> > > > >
> > > > > On Mon, Mar 27, 2017 at 2:24 PM, Sailesh Mukil <
> sail...@cloudera.com
> > >
> > > > > wrote:
> > > > > > On Mon, Mar 27, 2017 at 11:49 AM, Marcel Kornacker <
> > > > mar...@cloudera.com>
> > > > > > wrote:
> > > > > >
> > > > > >> On Mon, Mar 27, 2017 at 11:42 AM, Sailesh Mukil <
> > > sail...@cloudera.com
> > > > >
> > > > > >> wrote:
> > > > > >> > I will be working on a patch to add min/max filter support in
> > > > Impala,
> > > > > and
> > > > > >> > as a first step, specifically target the KuduScanNode, since
> the
> > > > Kudu
> > > > > >> > client is already able to accept a Min and a Max that it would
> > > > > internally
> > > > > >> > use to filter during its scans. Below is a brief design
> > proposal.
> > > > > >> >
> > > > > >> > *Goal:*
> > > > > >> >
> > > > > >> > To leverage runtime min/max filter support in Kudu for the
> > > potential
> > > > > >> speed
> > > > > >> > up of queries over Kudu tables. Kudu does this by taking a min
> > > and a
> > > > > max
> > > > > >> > that Impala will provide and only return values in the range
> > > Impala
> > > > is
> > > > > >> > interested in.
> > > > > >> >
> > > > > >> > *[min <= range we're interested in >= max]*
> > > > > >> >
> > > > > >> > *Proposal:*
> > > > > >> >
> > > > > >> >
> > > > > >> >- As a first step, plumb the runtime filter code from
> > > > > >> > *ex

Re: Min/Max runtime filtering on Impala-Kudu

[Sailesh Mukil]

On Tue, Mar 28, 2017 at 3:33 AM, Lars Volker <l...@cloudera.com> wrote:

> Thanks for sending this around, Sailesh.
>
> parquet-column-stats.h is somewhat tied to parquet statistics and will soon
> need more parquet-specific fields (null_count and distinct_count). It will
> also need extension to copy strings and handle there memory when tracking
> min/max values for variable length data across row batches. I'm not sure
> how these changes will affect its suitability for your purposes, but I
> thought I'd give a quick heads up.
>
> Thanks, Lars. In that case, it looks like the right path to take is to
implement a simpler version of this for this patch. Thanks for weighing in.

On Mon, Mar 27, 2017 at 11:35 PM, Todd Lipcon <t...@cloudera.com> wrote:
>
> > Sounds reasonable to me as well.
> >
> > The only thing I'd add is that, if it's easy to design the code to be
> > extended to pushing small 'IN (...)' predicate for low-cardinality
> filters,
> > that would be great. eg if the filter can start as an IN(...) and then if
> > it exceeds 32K (or whatever arbitrary threshold), "collapse" it to the
> > min/max range predicate?
> >
> > This should have a big advantage for partition pruning in low-cardinality
> > joins against hash-partitioned tables.
> >
> > -Todd
> >
>

Thanks, Todd. You're right, the IN predicate filters would be a great
addition. We can add that as a follow on patch, just to keep this patch
simple for now.

>
> >
> > On Mon, Mar 27, 2017 at 2:29 PM, Matthew Jacobs <m...@cloudera.com> wrote:
> >
> > > Thanks for writing this up, Sailesh. It sounds reasonable.
> > >
> > > On Mon, Mar 27, 2017 at 2:24 PM, Sailesh Mukil <sail...@cloudera.com>
> > > wrote:
> > > > On Mon, Mar 27, 2017 at 11:49 AM, Marcel Kornacker <
> > mar...@cloudera.com>
> > > > wrote:
> > > >
> > > >> On Mon, Mar 27, 2017 at 11:42 AM, Sailesh Mukil <
> sail...@cloudera.com
> > >
> > > >> wrote:
> > > >> > I will be working on a patch to add min/max filter support in
> > Impala,
> > > and
> > > >> > as a first step, specifically target the KuduScanNode, since the
> > Kudu
> > > >> > client is already able to accept a Min and a Max that it would
> > > internally
> > > >> > use to filter during its scans. Below is a brief design proposal.
> > > >> >
> > > >> > *Goal:*
> > > >> >
> > > >> > To leverage runtime min/max filter support in Kudu for the
> potential
> > > >> speed
> > > >> > up of queries over Kudu tables. Kudu does this by taking a min
> and a
> > > max
> > > >> > that Impala will provide and only return values in the range
> Impala
> > is
> > > >> > interested in.
> > > >> >
> > > >> > *[min <= range we're interested in >= max]*
> > > >> >
> > > >> > *Proposal:*
> > > >> >
> > > >> >
> > > >> >- As a first step, plumb the runtime filter code from
> > > >> > *exec/hdfs-scan-node-base.cc/h
> > > >> ><http://hdfs-scan-node-base.cc/h>* to *exec/scan-node.cc/h
> > > >> ><http://scan-node.cc/h>*, so that it can be applied to
> > > *KuduScanNode*
> > > >> >cleanly as well, since *KuduScanNode* and *HdfsScanNodeBase*
> both
> > > >> >inherit from *ScanNode.*
> > > >>
> > > >> Quick comment: please make sure your solution also applies to
> > > >> KuduScanNodeMt.
> > > >>
> > > >
> > > > Thanks for the input, I'll make sure to do that.
> > > >
> > > >
> > > >>
> > > >> >- Reuse the *ColumnStats* class (exec/parquet-column-stats.h)
> or
> > > >> >implement a lighter weight version of it to process and store
> the
> > > Min
> > > >> and
> > > >> >the Max on the build side of the join.
> > > >> >- Once the Min and Max values are added to the existing runtime
> > > filter
> > > >> >structures, as a first step, we will ignore the Min and Max
> > values
> > > for
> > > >> >non-Kudu tables. Using them for non-Kudu tables can come in as
> a
> > > >> following
> > > >> >patch(es).
> > > >> >- Similarly, the bloom filter will be ignored for Kudu tables,
> > and
> > > >> only
> > > >> >the Min and Max values will be used, since Kudu does not accept
> > > bloom
> > > >> >filters yet. (https://issues.apache.org/
> jira/browse/IMPALA-3741)
> > > >> >- Applying the bloom filter on the Impala side of the Kudu scan
> > > (i.e.
> > > >> in
> > > >> >KuduScanNode) is not in the scope of this patch.
> > > >> >
> > > >> >
> > > >> > *Complications:*
> > > >> >
> > > >> >- We have to make sure that finding the Min and Max values on
> the
> > > >> build
> > > >> >side doesn't regress certain workloads, since the difference
> > > between
> > > >> >generating a bloom filter and generating a Min and a Max, is
> > that a
> > > >> bloom
> > > >> >filter can be type agnostic (we just take a raw hash over the
> > data)
> > > >> whereas
> > > >> >a Min and a Max have to be type specific.
> > > >>
> > >
> >
> >
> >
> > --
> > Todd Lipcon
> > Software Engineer, Cloudera
> >
>

Re: Min/Max runtime filtering on Impala-Kudu

[Sailesh Mukil]

On Mon, Mar 27, 2017 at 11:49 AM, Marcel Kornacker <mar...@cloudera.com>
wrote:

> On Mon, Mar 27, 2017 at 11:42 AM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
> > I will be working on a patch to add min/max filter support in Impala, and
> > as a first step, specifically target the KuduScanNode, since the Kudu
> > client is already able to accept a Min and a Max that it would internally
> > use to filter during its scans. Below is a brief design proposal.
> >
> > *Goal:*
> >
> > To leverage runtime min/max filter support in Kudu for the potential
> speed
> > up of queries over Kudu tables. Kudu does this by taking a min and a max
> > that Impala will provide and only return values in the range Impala is
> > interested in.
> >
> > *[min <= range we're interested in >= max]*
> >
> > *Proposal:*
> >
> >
> >- As a first step, plumb the runtime filter code from
> > *exec/hdfs-scan-node-base.cc/h
> ><http://hdfs-scan-node-base.cc/h>* to *exec/scan-node.cc/h
> ><http://scan-node.cc/h>*, so that it can be applied to *KuduScanNode*
> >cleanly as well, since *KuduScanNode* and *HdfsScanNodeBase* both
> >inherit from *ScanNode.*
>
> Quick comment: please make sure your solution also applies to
> KuduScanNodeMt.
>

Thanks for the input, I'll make sure to do that.


>
> >- Reuse the *ColumnStats* class (exec/parquet-column-stats.h) or
> >implement a lighter weight version of it to process and store the Min
> and
> >the Max on the build side of the join.
> >- Once the Min and Max values are added to the existing runtime filter
> >structures, as a first step, we will ignore the Min and Max values for
> >non-Kudu tables. Using them for non-Kudu tables can come in as a
> following
> >patch(es).
> >- Similarly, the bloom filter will be ignored for Kudu tables, and
> only
> >the Min and Max values will be used, since Kudu does not accept bloom
> >filters yet. (https://issues.apache.org/jira/browse/IMPALA-3741)
> >- Applying the bloom filter on the Impala side of the Kudu scan (i.e.
> in
> >KuduScanNode) is not in the scope of this patch.
> >
> >
> > *Complications:*
> >
> >- We have to make sure that finding the Min and Max values on the
> build
> >side doesn't regress certain workloads, since the difference between
> >generating a bloom filter and generating a Min and a Max, is that a
> bloom
> >filter can be type agnostic (we just take a raw hash over the data)
> whereas
> >a Min and a Max have to be type specific.
>

Min/Max runtime filtering on Impala-Kudu

[Sailesh Mukil]

I will be working on a patch to add min/max filter support in Impala, and
as a first step, specifically target the KuduScanNode, since the Kudu
client is already able to accept a Min and a Max that it would internally
use to filter during its scans. Below is a brief design proposal.

*Goal:*

To leverage runtime min/max filter support in Kudu for the potential speed
up of queries over Kudu tables. Kudu does this by taking a min and a max
that Impala will provide and only return values in the range Impala is
interested in.

*[min <= range we're interested in >= max]*

*Proposal:*


   - As a first step, plumb the runtime filter code from
*exec/hdfs-scan-node-base.cc/h
   * to *exec/scan-node.cc/h
   *, so that it can be applied to *KuduScanNode*
   cleanly as well, since *KuduScanNode* and *HdfsScanNodeBase* both
   inherit from *ScanNode.*
   - Reuse the *ColumnStats* class (exec/parquet-column-stats.h) or
   implement a lighter weight version of it to process and store the Min and
   the Max on the build side of the join.
   - Once the Min and Max values are added to the existing runtime filter
   structures, as a first step, we will ignore the Min and Max values for
   non-Kudu tables. Using them for non-Kudu tables can come in as a following
   patch(es).
   - Similarly, the bloom filter will be ignored for Kudu tables, and only
   the Min and Max values will be used, since Kudu does not accept bloom
   filters yet. (https://issues.apache.org/jira/browse/IMPALA-3741)
   - Applying the bloom filter on the Impala side of the Kudu scan (i.e. in
   KuduScanNode) is not in the scope of this patch.


*Complications:*

   - We have to make sure that finding the Min and Max values on the build
   side doesn't regress certain workloads, since the difference between
   generating a bloom filter and generating a Min and a Max, is that a bloom
   filter can be type agnostic (we just take a raw hash over the data) whereas
   a Min and a Max have to be type specific.

Re: Join request

[Sailesh Mukil]

Hi Mahesh,

Thanks for you interest! You can get started by having a look at the
Contribution guidelines:
https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala

It walks you through how to get started on your first patch. Let us know if
you have any questions.

- Sailesh

On Wed, Mar 8, 2017 at 6:52 AM, Mahesh Balija 
wrote:

> Hi,
>
> I would like to contribute to Impala project.
>
> Best
> Mahesh
>

[Toolchain-CR] Bump Kudu version to latest master (cd7b0dd)

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Bump Kudu version to latest master (cd7b0dd)
..


Patch Set 1: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcd4dbb4b7e3c1ddd8c85b00ec443f7289dc1617
Gerrit-PatchSet: 1
Gerrit-Project: Toolchain
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

Re: Impala Hbase Security

[Sailesh Mukil]

https://www.cloudera.com/documentation/enterprise/5-8-x/topics/impala_hbase.html

The Impala docs just specify: "For details about HBase security, see
the Security
chapter in the HBase Reference Guide
."

That link has a section on "Client-side Configuration for Secure
Operation" which
can be tried to see if it works. It should work if configured correctly but
it's hard to tell as AFAIK, our testing does not include Impala over HBase
in a secure environment.

On Thu, Feb 2, 2017 at 10:34 AM, Tim Armstrong 
wrote:

> Does anyone on dev@ about this? I'm guess we don't support impersonation
> but have no idea if we support kerberos - is that automatically picked up
> by the HBASE client?
>
> On Thu, Feb 2, 2017 at 6:55 AM, Danny Morgan 
> wrote:
>
> > Hi Everyone, any luck?
> > --
> > *From:* Danny Morgan 
> > *Sent:* Friday, January 27, 2017 10:08:12 PM
> > *To:* u...@impala.incubator.apache.org
> > *Subject:* Impala Hbase Security
> >
> >
> > Does Impala support HBase security? Can Impala impersonation end users
> when
> >  access HBase?
> >
> >
> > Does Impala work with Kerberized HBase?
> >
> >
> > Thank You
> >
> >
>

Re: random query generator patch submit request

[Sailesh Mukil]

Just did a push_to_asf now, so it should be in sync.

On Wed, Nov 30, 2016 at 8:30 AM, Michael Brown <mi...@cloudera.com> wrote:

> Thanks!
>
> While you're at it, asf/master needs to be brought up to date with
> asf_gerrit/master:
>
> * 585ed5a - (asf_gerrit/master) IMPALA-4450: qgen: use string concatenation
> operator for postgres queries (7 minutes ago) 
> * 0f62bf3 - IMPALA-4550: Fix CastExpr analysis for substituted slots (5
> hours ago) 
> * 9f497ba - IMPALA-2890: Support ALTER TABLE statements for Kudu tables (12
> hours ago) 
> * 90bf40d - IMPALA-4553: ntpd must be synchronized for kudu to start. (16
> hours ago) 
> * 2680580 - (asf/master) IMPALA-4512: Add a script that builds Impala on
> stock Ubuntu 14.04. (18 hours ago) 
>
>
> On Wed, Nov 30, 2016 at 8:23 AM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
>
> > Done.
> >
> > On Wed, Nov 30, 2016 at 8:07 AM, Michael Brown <mi...@cloudera.com>
> wrote:
> >
> > > Hello,
> > >
> > > This is a patch to the random query generator, not affected by GVO:
> > >
> > > https://gerrit.cloudera.org/#/c/5034/
> > >
> > > Since GVO doesn't touch this code path, and Jenkins executors are
> > precious,
> > > could a Committer please verify/submit this change directly?
> > >
> > > Thanks
> > >
> >
>

Re: random query generator patch submit request

[Sailesh Mukil]

Done.

On Wed, Nov 30, 2016 at 8:07 AM, Michael Brown  wrote:

> Hello,
>
> This is a patch to the random query generator, not affected by GVO:
>
> https://gerrit.cloudera.org/#/c/5034/
>
> Since GVO doesn't touch this code path, and Jenkins executors are precious,
> could a Committer please verify/submit this change directly?
>
> Thanks
>

Re: Tests that don't run impalad

[Sailesh Mukil]

On Tue, Nov 29, 2016 at 9:50 AM, Henry Robinson  wrote:

> On 29 November 2016 at 08:06, Jim Apple  wrote:
>
> > Should we add to our pre-merge testing (aka GVM, aka GVO) some tests
> > that don't run impalad, but only build it or check for correctness?
> >
> > For instance:
> >
> > 1. bin/run_clang_tidy.sh - should we force our code to always be
> > clang-tidy?
> >
>
> I don't have enough experience of the tool to know if this likely to be a
> help or hindrance.
>
>
>
+1 for this. My opinion is unless we foresee some patches that would fail
clang-tidy but still be considered a valid patch by us, we should have this
as a pre-commit test.

>
> > 2. bin/check-rat-report.py - uses Apache RAT to check that our code
> > has proper license headers
> >
>
> +1
>
>
> >
> > 3. Other buildall.sh options - in the past we have broken -asan,
> > -release, or -so without breaking the pre-commit test.
> >
>
> If all can be tested for 'free' wrt to wall-clock-time, then sure. But if
> that's not possible, I'd only consider building -release, and maybe not
> even that. -asan failures are infrequent enough that I don't expect it to
> be worth the extra time it would add to the pre-commit build.
>
> -so is less important to me.
>
>
> >
> > 4. Docs build
> >
> > I think I can do these without increasing the end-to-end time it takes
> > to run the tests, by doing them in parallel. One issue I see is that
> > committers who see their change as minor and merge it manually,
> > without pre-merge testing, might break clang-tidy or RAT tests.
> >
>
> For that reason, perhaps a separate docs build makes the most sense.
>

Re: ASF crypto regulations

[Sailesh Mukil]

On Thu, Nov 3, 2016 at 5:14 PM, Todd Lipcon <t...@cloudera.com> wrote:

> On Thu, Nov 3, 2016 at 5:01 PM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
>
> > Just an FYI.
> >
> > Todd brought this to our attention.
> >
> > https://issues.cloudera.org/browse/IMPALA-4406
> > https://www.apache.org/dev/crypto.html
> >
> > Since Impala uses OpenSSL, we are required to follow the ASF crypto
> > regulations. It states that we have to add Apache Impala to their list of
> > Export projects:
> > http://www.apache.org/licenses/exports/
> > I will do this shortly.
> >
>
> I think you'll need PMC chair privileges to commit to change this file. But
> if you send me a diff against the XML file I can commit it on your behalf.
>
>
Thanks Todd! Just sent you a mail.


>
> >
> > It also requires the Incubator PMC chair to notify the US Government of
> the
> > same. (Step 3 in the above link)
> >
>
> Yea, I think once you generate it, you can send this to the
> incubator-general list and the chair will take care of resending it to the
> gov addresses.
>
>
>
I will send the generated mail to the list.


> >
> > We also need to add a notice in our codebase of the same. I've uploaded a
> > patch for this:
> > https://gerrit.cloudera.org/#/c/4940/
> >
> > - Sailesh
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

ASF crypto regulations

[Sailesh Mukil]

Just an FYI.

Todd brought this to our attention.

https://issues.cloudera.org/browse/IMPALA-4406
https://www.apache.org/dev/crypto.html

Since Impala uses OpenSSL, we are required to follow the ASF crypto
regulations. It states that we have to add Apache Impala to their list of
Export projects:
http://www.apache.org/licenses/exports/
I will do this shortly.

It also requires the Incubator PMC chair to notify the US Government of the
same. (Step 3 in the above link)

We also need to add a notice in our codebase of the same. I've uploaded a
patch for this:
https://gerrit.cloudera.org/#/c/4940/

- Sailesh

Re: Backporting error codes

[Sailesh Mukil]

Of the given choices, I would choose option 2. Not sure if there's a better
way. Maybe we could add a "// backported. not used" comment next to each
unused error code?

On Thu, Oct 20, 2016 at 4:24 PM, Lars Volker  wrote:

> Compilation fails if they are not: Numeric error codes must start from 0,
> be in order, and not have any gaps: got 94, expected 91
>
> Since I'm backporting a fix it feels dangerous to remove that restriction
> in the process.
>
> On Thu, Oct 20, 2016 at 4:16 PM, Henry Robinson 
> wrote:
>
> > How about not changing the code? Is there any reason they have to be
> > gapless?
> >
> > On 20 October 2016 at 16:12, Lars Volker  wrote:
> >
> > > When backporting a change that introduced a new error code to an older
> > > version Impala there seem to be two options to prevent gaps in the
> error
> > > codes:
> > >
> > >
> > >- Change the error code number during the backport. This will result
> > in
> > >different error codes between versions
> > >- Backport all new error codes that have been introduced prior to
> that
> > >change, so that the error code stays the same.
> > >
> > > Are there other alternatives? Which way should I go?
> > >
> > > Thanks, Lars
> > >
> >
> >
> >
> > --
> > Henry Robinson
> > Software Engineer
> > Cloudera
> > 415-994-6679
> >
>

Re: How do I turn up HDFS logging?

[Sailesh Mukil]

Thanks!

On Mon, Oct 3, 2016 at 7:43 PM, Juan <any...@gmail.com> wrote:

> set "non_impala_java_vlog" startup flag to 2 or 3
>
> On Mon, Oct 3, 2016 at 1:04 PM, Sailesh Mukil <sail...@cloudera.com>
> wrote:
>
> > Hi,
> >
> > Is there any option to turn up the level of HDFS logging and have it
> > redirected to the Impala logs?
> >
> > - Sailesh
> >
>

How do I turn up HDFS logging?

[Sailesh Mukil]

Hi,

Is there any option to turn up the level of HDFS logging and have it
redirected to the Impala logs?

- Sailesh

Re: Can you please send the output of these commands?

[Sailesh Mukil]

 sailesh@ubuntu:~$ cat /etc/*release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=14.04
DISTRIB_CODENAME=trusty
DISTRIB_DESCRIPTION="Ubuntu 14.04.2 LTS"
NAME="Ubuntu"
VERSION="14.04.2 LTS, Trusty Tahr"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 14.04.2 LTS"
VERSION_ID="14.04"
HOME_URL="http://www.ubuntu.com/;
SUPPORT_URL="http://help.ubuntu.com/;
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/;

sailesh@ubuntu:~$ uname -a
Linux ubuntu 3.16.0-45-generic #60~14.04.1-Ubuntu SMP Fri Jul 24 21:16:23
UTC 2015 x86_64 x86_64 x86_64 GNU/Linux

sailesh@ubuntu:~$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq
400

sailesh@ubuntu:~$ head -30 /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 60
model name : Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
stepping : 3
microcode : 0x1c
cpu MHz : 3600.000
cache size : 8192 KB
physical id : 0
siblings : 8
core id : 0
cpu cores : 4
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat
pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology
nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx smx
est tm2 ssse3 fma cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic movbe popcnt
tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm ida arat epb
xsaveopt pln pts dtherm tpr_shadow vnmi flexpriority ept vpid fsgsbase
tsc_adjust bmi1 avx2 smep bmi2 erms invpcid
bogomips : 7183.16
clflush size : 64
cache_alignment : 64
address sizes : 39 bits physical, 48 bits virtual
power management:

processor : 1
vendor_id : GenuineIntel
cpu family : 6

model : 60


On Fri, Sep 23, 2016 at 10:35 AM, Lars Volker  wrote:

> For IMPALA-4193  I'm
> trying
> to see how widely supported the cpufreq subsystem of sysfs is. If would
> help a lot if you could run the following commands on your development
> machine and send the output to me. Thanks, Lars
>
> cat /etc/*release
> uname -a
> cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq
> head -30 /proc/cpuinfo
>
> As an example, here's the output from my dev machine:
>
> ✔ ~$ cat /etc/*release
> DISTRIB_ID=Ubuntu
> DISTRIB_RELEASE=14.04
> DISTRIB_CODENAME=trusty
> DISTRIB_DESCRIPTION="Ubuntu 14.04.4 LTS"
> NAME="Ubuntu"
> VERSION="14.04.4 LTS, Trusty Tahr"
> ID=ubuntu
> ID_LIKE=debian
> PRETTY_NAME="Ubuntu 14.04.4 LTS"
> VERSION_ID="14.04"
> HOME_URL="http://www.ubuntu.com/;
> SUPPORT_URL="http://help.ubuntu.com/;
> BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/;
> ✔ ~$ uname -a
> Linux lv-desktop 4.2.0-27-generic #32~14.04.1-Ubuntu SMP Fri Jan 22
> 15:32:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
> ✔ ~$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq
> 400
> ✔ ~$ head -30 /proc/cpuinfo
> processor   : 0
> vendor_id   : GenuineIntel
> cpu family  : 6
> model   : 60
> model name  : Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
> stepping: 3
> microcode   : 0x1c
> cpu MHz : 799.875
> cache size  : 8192 KB
> physical id : 0
> siblings: 8
> core id : 0
> cpu cores   : 4
> apicid  : 0
> initial apicid  : 0
> fpu : yes
> fpu_exception   : yes
> cpuid level : 13
> wp  : yes
> flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca
> cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx
> pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl
> xtopology nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor
> ds_cpl vmx smx est tm2 ssse3 fma cx16 xtpr pdcm pcid sse4_1 sse4_2 x2apic
> movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm ida
> arat epb pln pts dtherm tpr_shadow vnmi flexpriority ept vpid fsgsbase
> tsc_adjust bmi1 avx2 smep bmi2 erms invpcid xsaveopt
> bugs:
> bogomips: 7183.48
> clflush size: 64
> cache_alignment : 64
> address sizes   : 39 bits physical, 48 bits virtual
> power management:
>
> processor   : 1
> vendor_id   : GenuineIntel
> cpu family  : 6
> ✔ ~$
>

[Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads
..

IMPALA-3823: Add timer to measure Parquet footer reads

It's been observed that Parquet footer reads perform poorly especially
when reading from S3. This patch adds a timer "FooterProcessingTimer"
which keeps a track of the average time each split of each scan node
spends in reading and processing the parquet footer.

Added a new utility counter called MinMaxAvgValueCounter which keeps
a track of the min, max and average values seen so far from a set of
values. This counter is used to calculate the min, max and average
time taken to scan and process Parquet footers per query per node.
This is also displayed in the RuntimeProfile.

The RuntimeProfile has also been updated to keep a track of, display
and move this new MinMaxAvgValueCounter between nodes through Thrift.

A test has been added to test that this counter works fine when there
are multiple blocks to scan per node.

Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_scanners.py
7 files changed, 216 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/4371/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4371

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads
..

IMPALA-3823: Add timer to measure Parquet footer reads

It's been observed that Parquet footer reads perform poorly especially
when reading from S3. This patch adds a timer "FooterProcessingTimer"
which keeps a track of the average time each split of each scan node
spends in reading and processing the parquet footer.

Added a new utility counter called MinMaxAvgValueCounter which keeps
a track of the min, max and average values seen so far from a set of
values. This counter is used to calculate the min, max and average
time taken to scan and process Parquet footers per query per node.
This is also displayed in the RuntimeProfile.

The RuntimeProfile has also been updated to keep a track of, display
and move this new MinMaxAvgValueCounter between nodes through Thrift.

A test has been added to test that this counter works fine when there
are multiple blocks to scan per node.

Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
M common/thrift/RuntimeProfile.thrift
M tests/query_test/test_scanners.py
7 files changed, 216 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/4371/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide 
execution state.
..

IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

This patch is a header preview of a query wide execution state for
a backend.

None of the existing headers are replaced, they are just added in
parallel to existing headers and not considered for compilation.

Changes in design are as follows:

FragmentExecState -> FInstanceState
  This is now what was previously the FragmentExecState and the
  PlanFragmentExecutor together. The ReportStatusCallback is removed
  and now SendReport() is in charge of sending the report directly.
  The callback was in place originally only so the PlanFragmentExecutor
  could call into the FragmentExecState without a reference. This is
  not necessary anymore.

PlanFragmentExecutor (collapsed into FInstanceState)

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryState.
  This class is responsible for cleaning up the QueryState.

QueryState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FInstanceStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryState.
  Every user of the QueryState must access it within the scope of a
  'Guard' object which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

Excludes:
 - All .cc files to make use of changes made.

Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
---
A be/src/runtime/fragment-instance-state.h
A be/src/runtime/query-state.h
M be/src/runtime/runtime-state.h
A be/src/service/query-exec-mgr.h
4 files changed, 587 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/4301/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide 
execution state.
..


Patch Set 1:

> > > (1 comment)
 > >
 > > @Matt:
 > >
 > > The RuntimeState has members that are relatively more frequently
 > > accessed than QueryState or FInstanceState members.
 > > So to have good cache temporal locality, we could leave it as a
 > > separate class.
 > >
 > > But to your point, it is a mess. Now that more people are
 > bringing
 > > this up, I feel having a RuntimeState per query wouldn't be a bad
 > > idea. We can move the fragment instance specific stuff to the
 > > FInstanceState and leave the query specific stuff in
 > RuntimeState.
 > >
 > > The QueryState on the other hand would just contain state
 > required
 > > for execution which relatively isn't accessed that frequently.
 > >
 > > Thoughts?
 > 
 > Oh right, now I remember that cache locality had been brought up,
 > but I think we can probably work around it. (I guess one of the
 > issues is that there are larger structures, e.g. thrift
 > query/fragment descriptors? Those we can store on the heap, e.g.
 > via a scoped_ptr/unique_ptr on the QueryState/FInstanceState.) IMO
 > the RuntimeState is pretty gross and messes up your nice new
 > abstractions, so I think it'd be worth us chatting more about.
 > Perhaps we can chat in person next wk?

Sure! Let's chat in person next week.

-- 
To view, visit http://gerrit.cloudera.org:8080/4301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 14:

(13 comments)

All these comments addressed in:
https://gerrit.cloudera.org/#/c/4306/

http://gerrit.cloudera.org:8080/#/c/4066/11/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 380:   for (; entry != _TExecNodePhase_VALUES_TO_NAMES.end(); ++entry) {
> the QueryState tear-down will not happen until the refcount is 0, and the r
Done


http://gerrit.cloudera.org:8080/#/c/4066/12/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS12, Line 272: boost
> remove boost::
Done


PS12, Line 275: std
> remove std::
Done


PS12, Line 296: std
> remove std::
Done


PS12, Line 299: boost
> remove boost::
Done


PS12, Line 313: local
> local?
Removed 'local'.


Line 324: 
> remove blank line
Done


PS12, Line 369: // This may be NULL while executing UDFs.
  :   if (filter_mem_tracker_.get() != nullptr) {
  : filter_mem_tracker_->UnregisterFromParent();
  :   }
> Why not do this in TearDown()?
Done


Line 2074: DCHECK(state->disabled() || state->pending_count() == 0);
> This just DCHECKS the previous line (except covering the case where pending
Probably not. Removed it.


PS12, Line 2091: 
filter_mem_tracker_->Release(aggregated_filter->directory.size());
> nit: shouldn't you release *after* the swap()? Won't make any difference in
We could, but I just thought it would be more readable this way because if we 
do it after, aggregated_filter->directory.size() will be 0. And we will have to 
release it from rpc_params instead which could confuse readers.


PS12, Line 2100: Disable
> Doesn't this mean that every filter eventually gets a 'true' in its Disable
Yes, fixed this in the follow up patch.


PS12, Line 2133: Disable(coord->filter_mem_tracker_.get());
> not clear - is this branch only taken if the filter is partitioned only? Ot
Yes, fixed it in the follow up patch.


http://gerrit.cloudera.org:8080/#/c/4066/13/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS13, Line 306: Only set for partitioned joins
> is this true? i think the latest revisions made this a false statement, no?
Yes, but now the follow-up fix is back to this case. So leaving it as it is.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..

IMPALA-4053: Address follow up comments for IMPALA-3610

This is a follow up to the 'Account memory for global runtime filter
aggregation patch'.

This follow up contains:
 * Broadcast filter updates don't TryConsume() memory anymore and are
   sent immediately. (So the first one will always succeed, subsequent
   ones will be ignored)
 * 'disable' filter happens only for error cases. They are discarded
   otherwise.
 * Some style changes.

Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
---
M be/src/runtime/coordinator.cc
1 file changed, 62 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/4306/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4053: Address follow up comments for IMPALA-3610

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4306

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610
..

IMPALA-4053: Address follow up comments for IMPALA-3610

This is a follow up to the 'Account memory for global runtime filter
aggregation patch'.

This follow up contains:
 * Broadcast filter updates don't TryConsume() memory anymore and are
   sent immediately. (So the first one will always succeed, subsequent
   ones will be ignored)
 * 'disable' filter happens only for error cases. They are discarded
   otherwise.
 * Some style changes.

Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
---
M be/src/runtime/coordinator.cc
1 file changed, 62 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/4306/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide 
execution state.
..


Patch Set 1:

This addresses all the header comments in the non-header only patch:
(i.e. https://gerrit.cloudera.org/#/c/3817/)

I will post a compilable version of this patch in that CR. That will be a lot 
more tricky mainly because:
 - PlanFragmentExecutor and FragmentExecState are now one class. This makes the 
coordinator code tricky because the Coordinator had special code which depended 
only on the PFE and not on the FES; and has redundant FES functionality to 
manage the PFE.

@Lars: This patch can be used as a guide for the per-query RPC (IMPALA-2550), 
and also for working out the final kinks in the headers.

-- 
To view, visit http://gerrit.cloudera.org:8080/4301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4301

Change subject: IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide 
execution state.
..

IMPALA-4014: PARALLEL HEADERS ONLY: Introduce query-wide execution state.

This patch is a header preview of a query wide execution state for
a backend.

None of the existing headers are replaced, they are just added in
parallel to existing headers and not considered for compilation.
Compiling the code will work and is no different from the current
apache/master branch (i.e. the commit before this patch).

Changes in design are as follows:

FragmentExecState -> FInstanceState
  This is now what was previously the FragmentExecState and the
  PlanFragmentExecutor together. The ReportStatusCallback is removed
  and now SendReport() is in charge of sending the report directly.
  The callback was in place originally only so the PlanFragmentExecutor
  could call into the FragmentExecState without a reference. This is
  not necessary anymore.

PlanFragmentExecutor (collapsed into FInstanceState)

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryState.
  This class is responsible for cleaning up the QueryState.

QueryState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FInstanceStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryState.
  Every user of the QueryState must access it within the scope of a
  'Guard' object which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

runtime-state-new.h is what runtime-state.h would look like, and is
just a temporary name.

Excludes:
 - All .cc files to make use of changes made.

Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
---
A be/src/runtime/fragment-instance-state.h
A be/src/runtime/query-state.h
A be/src/runtime/runtime-state-new.h
A be/src/service/query-exec-mgr.h
4 files changed, 981 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/01/4301/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4301
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If58292a5c377660d97e7e7cc0c3122328eba72ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

Re: Sending Code Review emails to a different list

[Sailesh Mukil]

+1

On Thu, Sep 1, 2016 at 4:11 PM, Daniel Hecht  wrote:

> Sounds good to me.
>
> On Thu, Sep 1, 2016 at 4:04 PM, Alex Behm  wrote:
>
> > Feels much better to have a separate cr list. +1 for this idea
> >
> > On Thu, Sep 1, 2016 at 3:57 PM, Jim Apple  wrote:
> >
> > > Today, all code reviews get send to dev@, causing it to have a huge
> > > mail volume (1777 messages in August). I feel dev@ might be more
> > > welcoming if it only included mails sent by humans; that way, new
> > > contributors don't have to worry about setting up filters right away.
> > > CR emails could go to code-rev...@impala.incubator.apache.org or
> > > something else that the Apache infra people are willing to set up.
> > >
> > > What does everyone else think?
> > >
> >
>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 13:

> > (12 comments)
 > >
 > > Mostly style comments, but one question about disabling broadcast
 > > joins too early.
 > 
 > Where are we on this?  Do we think we need to hold up the change?

Spoke to Henry about this and he said we can address it as a separate patch 
later and that we should continue with the GVO.

-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 13: Code-Review+2

Rebase. Carry +2.

-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3912: test random rpc timeout is flaky.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3912: test_random_rpc_timeout is flaky.
..


Patch Set 1: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4080
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I19f8b3fea66c5a0398e3476a46f060be9f951983
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Juan Yu <j...@cloudera.com>
Gerrit-Reviewer: Juan Yu <j...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Hello Marcel Kornacker,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4066

to look at the new patch set (#12).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 309 insertions(+), 101 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/12
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 11: Code-Review+2

(6 comments)

Carry +2

http://gerrit.cloudera.org:8080/#/c/4066/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 2110: DCHECK_EQ(state->bloom_filter(), 
reinterpret_cast<TBloomFilter*>(NULL));
> move this todo
Done


http://gerrit.cloudera.org:8080/#/c/4066/11/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 281: 
> you can remove all blank lines between the getters
Done


Line 380: // TODO-MT: remove
> Can you remove this comment?
Done


Line 380: // TODO-MT: remove
> there shouldn't be references to any query-related state when the QueryStat
The scanner threads would not know if an error has been hit on the main thread, 
so they would still be accessing query state.


Line 2098:   // Complete partitioned or broadcast join filter case.
> "complete filter case"
Done


Line 2133:   //if (desc_.is_broadcast_join) return;
> remove
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#11).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 319 insertions(+), 101 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 9:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/4066/9/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 262: /// A filter may be discarded if an always_true filter update is 
received, an OOM is hit,
> you use both disabled and discarded to mean the same thing (i think). let's
Done


Line 303:   /// Resets bloom_filter_ to NULL, sets pending_count_ to 0, 
releases any memory
> don't reference internal state in the comment of a public function (the rea
Done


Line 390: // comprehensive logging of all mem-trackers.
> add TODO-MT: remove
Done


Line 718: // Cast the const away to read non-const member functions. No 
modification is done.
> why do you need non-const access?
I can't call non-const functions otherwise, specifically state.targets() and 
state.src_fragment_instance_idxs(). They need to be non-const because they are 
written to elsewhere.

Is there a better way to do this?


Line 726: for (const auto& target: *state.targets()) {
> no auto
Done


Line 2048:   for (auto& filter: filter_routing_table_) {
> why make this a ref?
I don't understad, versus?
Not having a ref would implicitly call a copy-constructor.


Line 2086: DCHECK_GT(state->pending_count(), 0);
> from l2086 to l2090 can also go into ApplyUpdate()
Done


Line 2099: // If the filter was discarded, we should send out an 
always_true filter immediately.
> this comment doesn't describe the following statement
Done. Removed the comment. The next line describes this anyway.


Line 2105: // No more filters are pending on this filter ID. Create a 
distribution payload and
> "no more updates are..."
Done


Line 2107: state->set_completion_time(query_events_->ElapsedTime());
> move into ApplyUpdate
This has to be set only once we are sure that we aren't going to wait for 
anymore updates. So this should be set only once after all updates are received 
or if a filter is disabled.


Line 2119: if (state->desc().is_broadcast_join) {
> also swap in the filter in ApplyUpdate for broadcast joins, then you don't 
That would cause broadcast filters having to swap() twice, which is two times 
the memory copy which could be noticeable for large filters. Also, we would 
TryConsume() memory for broadcast filters too then.

It's probably okay since we process only one update for broadcast filters, but 
if we're okay with that I'll make the change quickly.


Line 2183:   pending_count_ = 0L;
> does the pending count still matter at this point (and if so, why?)
Not really. Setting it to 0 here has no more meaning. I removed this assignment.


http://gerrit.cloudera.org:8080/#/c/4066/9/be/src/util/bloom-filter-test.cc
File be/src/util/bloom-filter-test.cc:

Line 277:   BloomFilter::ToThrift(, _thrift);  
> trailing spaces
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#10).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 328 insertions(+), 100 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#9).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 331 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/9
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4066/7/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 2119: if (state->desc().is_broadcast_join) {
> this comment only applies to line 2122, right? If so, it'd be better to mov
Done


Line 2122:   _cast<TBloomFilter&>(params.bloom_filter);
> no need to break up the decl and assignment, even if it doesn't fit on a si
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#8).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 331 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 5:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/4066/5/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 284:   /// coarsley for every filter update.
> remove "coarsley" (sic)
Done


Line 289:   // Apply an update to the aggregated filter with the received 
filter.
> describe role of params
Done


Line 292:   TPublishFilterParams* rpc_params, const TUniqueId& query_id);
> in/out params go at end
I removed the out params. Done.


Line 343:   // logging.
> what do the scanner threads do with this mem tracker?
When a MemLimitExceeded error is hit, MemTracker::LogUsage() is called which 
touches all the mem-trackers of that query using the parent-child relationship 
and gives a report of memory usage up to that point.

If the coordinator is torn down before the scanner threads (which happens a lot 
apparently), the scanner threads will end up touching this already destroyed 
filter.


Line 2102: TPublishFilterParams* rpc_params, const TUniqueId& query_id) {
> pass in Coordinator* instead of the mem tracker and query_id, that's more c
Done


Line 2103:   filter_updates_received->Add(1);
> move this out into the caller, there is no other use of this parameter (and
Done


Line 2105:   if (!desc.is_broadcast_join) {
> if (is_broadcast_join) return;
Done


Line 2115: disabled = true;
> why don't you move this into Discard and then check that explicitly in Upda
Done


Line 2131:   // 'bloom_filter' will process subsequent updates otherwise.
> the reason this wasn't apparent is that it wasn't stated what a discarded/i
I moved disabled_ = true here, so that it's not so subtle what constitutes as a 
discarded filter.
Also updated comments to reflect what a discarded filter looks like.


http://gerrit.cloudera.org:8080/#/c/4066/6/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 2066: TBloomFilter* non_const_filter;
> rather than defining non_const_filter temporary here, I think it would be c
Done


PS6, Line 2068: non_const_filter
> this shadows the def at L2066, but as I said above, let's get rid of 2066. 
Done


Line 2074:   non_const_filter = state->bloom_filter.get();
> And then here:
Done


Line 2085: state->DiscardFilter(filter_mem_tracker_.get());
> Does this do anything in cases #1 and #3, or is it only for case #2 (aggreg
It's essentially a no-op for cases #1 and #3. I thought of moving it to L2077 
but I felt it was more readable this way.


http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:

Line 82:   void Or(const BloomFilter& other);
> in that case, please remove it.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#7).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
11 files changed, 331 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
..


Patch Set 11:

>From the discussion today:

Points agreed on:
 - Collapse Executors and states, i.e. collapse FragmentInstanceExecutor into 
FragmentInstanceState, so that responsibilities and boundaries of classes are 
clear.

 - QueryExecMgr is still responsible for setting up the QueryState and creating 
the fragment threads for now. Once the per-query RPC is implemented, the model 
will change such that QueryExecMgr creates a “query" thread which sets up the 
QueryState and starts the fragment threads.

Additionally, I'll put out a separate patch with *only* headers that are 
parallel with the existing headers, so that Lars can use that to work on the 
per-query RPC.

-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4066/5/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS5, Line 2121: rpc_params->bloom_filter
Corrected this mistake in the next patchset.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#6).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
10 files changed, 231 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#5).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
10 files changed, 231 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 4:

(10 comments)

I think I finally fixed the final crash. The lifetimes of every member 
introduced with this patch seemed to be perfect but ASAN still complained of a 
use-after-free issue.

I spoke to Alex, and he pointed out that the scanner threads that are spawned 
by the "main" thread may still be running after the Coordinator is destroyed. 
And that exactly was the issue. Thanks Alex.

The filter_mem_tracker_ introduced with this patch is the child tracker of the 
RuntimeState::query_mem_tracker_ in the case where there is a coordinator 
fragment. At times, the main thread exits cleanly, destroying the coordinator 
on an error (like OOM), but the scanner threads are still running and access 
MemTracker::LogUsage() which touch the Coordinator::filter_mem_tracker_ causing 
a use-after-free.

Looks like the lifetimes of many objects were not set concretely and we've been 
getting by without crashes due to shared pointers. Hopefully these lifetime 
issues will be fixed with the different patches that go in for the MT effort.

http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 2040: state->DiscardFilter(filter_mem_tracker_.get());
> initialize state->bloom_filter to an empty struct, then do the swap.
Done


http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 2010: lock_guard l(filter_lock_);
> this is a very coarse lock. most of the code after l2037 is specific to sta
I've left a TODO to add a per-object lock in FilterState.

I'm not sure how I can reduce the holding time of filter_lock_ without the per 
state lock itself.


Line 2037: state->ApplyUpdate(filter_updates_received_);
> almost everything from here to l2056 updates 'state'. move into ApplyUpdate
Done


Line 2051:   state->bloom_filter.reset(new 
TBloomFilter(params.bloom_filter));
> swap instead
Done


Line 2057: if (state->pending_count > 0) return;
> follow with blank line, there is a logical break here (first you updated st
Done


Line 2108:   pending_count = 0L;
> why did you move this assignment?
Yes it is necessary for correctness. I've added a comment why


http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/util/bloom-filter.cc
File be/src/util/bloom-filter.cc:

Line 104:   for (int i = 0; i < in.directory.size(); ++i) {
> single line?
Done


http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:

Line 82:   void Or(const BloomFilter& other);
> is this still getting called?
No it isn't. Only tests use it now.


Line 106:   static void OrTBloomFilter(const TBloomFilter& in, TBloomFilter* 
out);
> no need to repeat param types in function name.
Done


http://gerrit.cloudera.org:8080/#/c/4066/4/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
File 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test:

Line 85: SET MEM_LIMIT=200MB;
> can't this run with a smaller limit?
Did some empirical runs and found that I could run it with as low as 140MB.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 4:

(1 comment)

An Update: Ran it through ASAN and found an instance of use-after-free. Still 
trying to track down what exactly is going on.

http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS4, Line 474: filter_mem_tracker_
> all of the memory allocated for filters is specific to the query for which 
Sorry, my bad. Short lapse of confusion there. Yes it does belong to a 
particular query.

The runtime state's mem tracker is destroyed when the RuntimeState object 
itself is destroyed. That is destroyed when the executor_ is destroyed, and the 
executor_ is destroyed when the Coordinator object is destroyed.


So maybe this isn't a problem then.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4066/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS4, Line 474: filter_mem_tracker_
An issue here is that if there exists a coordinator fragment, this 
filter_mem_tracker_ will be the child of the 
Coordinator::executor_->runtime_state()->query_mem_tracker(), which will be 
destroyed before this filter_mem_tracker_ is destroyed, which could be the 
source of some problems.

Should we just make the filter_mem_tracker_ the child of the process mem 
tracker? Since it doesn't belong to any single query anyway.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
..


Patch Set 11:

> (2 comments)
 > 
 > As discussed in person earlier, we decided the best way forward
 > would be to split this into header changes + implementation (the
 > header changes will allow Lars to start working on the per-query
 > rpc independently).
 > 
 > We also decided to introduce parallel headers to the existing ones
 > (so we'll have FragmentInstanceState and FragmentExecState).

I'm currently stuck with trying to solve a crash for IMPALA-3610 and was 
dealing with an escalation over the weekend. Seeing as how we want to get in 
3610 for this release, I will focus on that for today and try to get back to 
this later tonight.

-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 4:

> (11 comments)

I stumbled across yet another crash over the weekend. I will try and fix that 
first and then address the comments.

-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#4).

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
10 files changed, 206 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/4066/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has abandoned this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Abandoned

Wrong change-id. Uploaded the right one after.

-- 
To view, visit http://gerrit.cloudera.org:8080/4143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I3ff5f2fcc55fac763cdb6c802acd9d02d3ee9877
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 2:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/4066/2//COMMIT_MSG
Commit Message:

Line 25: the output broadcast structure without copying.
> please add this explanation/invariant (which is very helpful for understand
Added explanation to the FilterState->bloom_filter member.


http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 431:   filter_mem_tracker_.reset(new MemTracker(-1, -1, "Runtime Filter 
(Coordinator)",
> move into c'tor, this is setup that can be done prior to execution.
The query_mem_tracker() is set only by L425 based on whether there exists a 
runtime state or not. So it needs to be done here.


Line 1965:   unordered_set target_fragment_instance_idxs;
> move to where it's used
If it's moved inside the curly braces, it will lose scope in L2051.


Line 1979: // Check if the filter has already been sent, which could happen 
in three cases: if
> turn into bullet points, easier to read
Done


Line 1995: --state->pending_count;
> turn this block into a member fn for state (ApplyUpdate?), that'll make it 
Done


Line 2006:   // Discard as one missing update means a correct filter 
cannot be produced.
> discard,
Done


Line 2007:   state->disabled = true;
> move into discard
'disabled' is just used to keep track of the cases where a filter is not used 
due to OOM. A filter will be discarded when it is complete, but that doesn't 
mean that it hit OOM.


Line 2013: // TODO: Implement BloomFilter::Or(const 
ThriftBloomFilter&)
> if we already have problems with memory spikes, shouldn't this be addressed
Done


Line 2026: for (const auto& target: state->targets) {
> remove auto unless it's an stl iterator
Done


Line 2035:   // move the payload from the request rather than copy it and 
take double the memory
> provide that information for the data structure in the .h file
Done


Line 2040:   swap(rpc_params->bloom_filter, *filter);
> why not do this for FilterState in the block above, rather than just the rp
Done.
We cannot however, do a swap() for the first oncoming non-broadcast filter 
because the state->bloom_filter will be NULL.


Line 2051:   for (const auto& target_idx: target_fragment_instance_idxs) {
> don't use auto here, this isn't an iterator
Done


Line 2054: auto cb = [rpc_params, addr = fragment_inst->impalad_address(),
> bind was more compact; please revert
Done


http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 195:   void Done();
> TearDown? done is very generic.
Done


Line 399: std::unique_ptr bloom_filter;
> is this why you're including the filter header?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4143

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..

IMPALA-3610: Account for memory used by filters in the coordinator

Before this patch, Impala would not account for the memory used to
aggregate runtime filters together in the coordinator. Impala's memory
could therefore be silently overcommitted.

This patch accounts for aggregated filter memory in a new filter
memtracker that is attached to the coordinator's query_mem_tracker(). If
the query memory limit is exceeded when a filter update arrives, that
update is discarded. If the filter is from a partitioned join, the
entire filter can therefore be discarded immediately (to alleviate
memory pressure) and a dummy 'always true' filter is sent to backends to
unblock them.

If the filter is from a broadcast join, no aggregation is done, so there
is no tracking. The Thrift input and output filter data structures are
not tracked (as we generally don't track RPC objects, but plan to in the
future). The filter payload is moved from the input request structure to
the output broadcast structure without copying.

Memory that is added to a memtracker must always be released. To do
this, we need to signal to the coordinator that it is finished, and that
there is no point trying to process any future updates that might arrive
concurrently. This patch adds Coordinator::Done() which is called from
QueryExecState::Done(), and which releases memory from all in-process
runtime filters.

Finally, this patch increases the upper limit for runtime filters to
512MB. This allows testing on very large datasets. The default maximum
is still 16MB, per RUNTIME_FILTER_MAX_SIZE.

Testing: Added a new test that triggers the OOM condition on the
coordinator. All existing runtime filter tests pass.

Change-Id: I3ff5f2fcc55fac763cdb6c802acd9d02d3ee9877
---
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-filter-bank.h
M be/src/service/impala-server.h
M be/src/service/query-exec-state.cc
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M 
testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters_phj.test
10 files changed, 206 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/4143/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4143
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3ff5f2fcc55fac763cdb6c802acd9d02d3ee9877
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#11).

Change subject: IMPALA-4014: Introduce query-wide execution state.
..

IMPALA-4014: Introduce query-wide execution state.

This patch is a header preview of a query wide execution state for
a backend.

Changes in design are as follows:

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryExecState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryExecState.
  This class is responsible for cleaning up the QueryExecState.

QueryExecState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FragmentExecStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryExecState.
  Every user of the QueryExecState must access it within the scope of
  ScopedQueryExecStateRef which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

QueryExecState (old) -> ClientRequestExecState:
  This is just a class name change.

  We still do not include shared state into the QueryExecState because
  there needs to be changes to DescriptorTbl, etc. before we can
  incorporate them into the QueryExecState. Will be done as separate
  patches.

P.S: This only shows what the changes would look like and as of yet
still does not compile.

Excludes:
 - Some class renames in places not important to the core patch logic.
 - Most .cc files to make use of changes made.

Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
---
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/exec-env.h
R be/src/runtime/fragment-instance-exec-state.cc
R be/src/runtime/fragment-instance-exec-state.h
R be/src/runtime/fragment-instance-executor.cc
R be/src/runtime/fragment-instance-executor.h
A be/src/runtime/query-exec-state.cc
A be/src/runtime/query-exec-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/CMakeLists.txt
R be/src/service/client-request-exec-state.cc
R be/src/service/client-request-exec-state.h
D be/src/service/fragment-mgr.cc
D be/src/service/fragment-mgr.h
M be/src/service/impala-internal-service.h
A be/src/service/query-exec-mgr.cc
A be/src/service/query-exec-mgr.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/ImpalaInternalService.thrift
23 files changed, 779 insertions(+), 385 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/11
-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
..


Patch Set 10:

(52 comments)

http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-exec-state.h
File be/src/runtime/fragment-instance-exec-state.h:

Line 32: /// Execution state of a single plan fragment.
> instance
Done


PS10, Line 33: FragmentInstanceExecState
> Let's consider FragmentInstanceState and QueryState (if they're not executi
Done


PS10, Line 33: FragmentInstanceExecState
> i think that's a good idea, long names aren't necessarily more readable or 
Done


PS10, Line 39: lambda(
 :   
boost::mem_fn(::ReportStatusCb),
 :   this, _1, _2, _3)
> Try and find a clearer way to write this. If you don't want to write the la
I'm still not very well versed on the C++ lambda  syntax.
My next step is to create a compilable version of this patch. I'll make the 
changes to this in the next patch set so that I get the syntax right and make 
sure that it compiles.
Also, I don't want to hold up reviewers while I figure this out.


PS10, Line 80: ImpalaBackendClientCache* client_cache_;
> Is this necessary given that ExecEnv can give us the same pointer?
No. I've changed it.


PS10, Line 77: QueryExecState* query_exec_state_;  // not owned
 :   TPlanFragmentInstanceCtx fragment_instance_ctx_;
 :   FragmentInstanceExecutor executor_;
 :   ImpalaBackendClientCache* client_cache_;
 :   TExecPlanFragmentParams exec_params_;
> Good opportunity to add some comments for these.
Done


PS10, Line 83: plan fragment
> fragment instance.
Done


http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.cc
File be/src/runtime/fragment-instance-executor.cc:

PS10, Line 80: Close
> Since we are aiming for deterministic, manual management of finst lifetimes
It already does get called explicitly and this call shouldn't be necessary. I 
changed it to a DCHECK.


Line 349: // We also want to call sink_->Close() here rather than in 
FragmentInstanceExecutor::Close
> long line
Done


http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/fragment-instance-executor.h
File be/src/runtime/fragment-instance-executor.h:

Line 48: /// FragmentInstanceExecutor handles all aspects of the execution of a 
single plan fragment,
> long line
Done


PS10, Line 155: ExecEnv* exec_env_;  // not owned
> Can you remove this? ExecEnv::GetInstance() works fine.
Done


http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.cc
File be/src/runtime/query-exec-state.cc:

PS10, Line 60:  
> remove space
Done


PS10, Line 73: 
DCHECK_EQ(fragment_inst_exec_state_map_.find(GetInstanceIdx(fragment_instance_id)),
 :   fragment_inst_exec_state_map_)
> these aren't the same type (does this compile?).
Oops, sorry. Meant to compare it with map_.end(). Changed it.

I haven't gotten this to compile yet, so there may be a few syntax errors that 
I've missed.


PS10, Line 76: FragmentInstanceExecState* fragment_inst_exec_state =
 :   new FragmentInstanceExecState(exec_params, this, 
ExecEnv::GetInstance());
> Presumably this should be managed by obj_pool. If not, the value type of th
Are there any arguments for or against a unique_ptr?

It seems to fit the use here perfectly:
- We would never want to transfer ownership of the FragmentInstanceState 
objects.
- We want its lifetime to be as long as the QueryState.

If there are no objections, I can change it to use a unique_ptr.


PS10, Line 80: his RPC returns
> This isn't an RPC any more. Update comment.
Done


PS10, Line 113: lexical_cast
> PrintId()
Done


PS10, Line 128: lexical_cast
> PrintId()
Done


http://gerrit.cloudera.org:8080/#/c/3817/10/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:

Line 38: /// This class is responsible for creating, running and destroying 
FragmentInstanceExecStates.
> Talk about lifetime here: when is this created, when is it destroyed, what 
Done


PS10, Line 41: // Special constructor for the coordinator.
> TODO: remove.
Done


PS10, Line 59: CleanFragmentInstanceStates
> Don't talk about implementation details (cleaning the map). When would this
Done


Line 61:   /// Registers a new FragmentInstanceExecState and launches the 
thread that calls Prepare() and
> long line.
Done


PS10, Line 90: std::unique_ptr<ObjectPool*> obj_pool_
> Comment broadly what this is used to manage, since that affects lifecycles.
It doesn't manage anything yet in this patch because we decided to bring in the 
shared state across fragments (DescriptorTbl) in a future patch.

The reason is that the serialization/deserialization of the DescriptorTbl needs 
to be modified quite a bit to become a general DescTbl ac

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS2, Line 2035: rather than copy it and take double the memory
  :   // cost
> Isn't this an issue for non broadcast join filters too though?
I'm okay with how it is now if there's no easy way to swap the memory**


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 2: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4066/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS2, Line 2035: rather than copy it and take double the memory
  :   // cost
Isn't this an issue for non broadcast join filters too though?

At least there we've reserved memory for one of the filters, and I'm okay with 
how it is now if there's no easy way to track the memory.


Line 2039:   TBloomFilter* filter = 
&(const_cast<TBloomFilter&>(params.bloom_filter));
Good call. Maybe it's worth adding the following in L2038 too:
DCHECK_EQ(state->bloom_filter.get(), NULL)


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#10).

Change subject: IMPALA-4014: Introduce query-wide execution state.
..

IMPALA-4014: Introduce query-wide execution state.

This patch is a header preview of a query wide execution state for
a backend.

Changes in design are as follows:

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryExecState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryExecState.
  This class is responsible for cleaning up the QueryExecState.

QueryExecState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FragmentExecStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryExecState.
  Every user of the QueryExecState must access it within the scope of
  ScopedQueryExecStateRef which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

QueryExecState (old) -> ClientRequestExecState:
  This is just a class name change.

  We still do not include shared state into the QueryExecState because
  there needs to be changes to DescriptorTbl, etc. before we can
  incorporate them into the QueryExecState. Will be done as separate
  patches.

P.S: This only shows what the changes would look like and as of yet
still does not compile.

Excludes:
 - Some class renames in places not important to the core patch logic.
 - Most .cc files to make use of changes made.

Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
---
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
R be/src/runtime/fragment-instance-exec-state.cc
R be/src/runtime/fragment-instance-exec-state.h
R be/src/runtime/fragment-instance-executor.cc
R be/src/runtime/fragment-instance-executor.h
A be/src/runtime/query-exec-state.cc
A be/src/runtime/query-exec-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/CMakeLists.txt
R be/src/service/client-request-exec-state.cc
R be/src/service/client-request-exec-state.h
D be/src/service/fragment-mgr.cc
D be/src/service/fragment-mgr.h
M be/src/service/impala-internal-service.h
A be/src/service/query-exec-mgr.cc
A be/src/service/query-exec-mgr.h
M common/thrift/ImpalaInternalService.thrift
20 files changed, 735 insertions(+), 360 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/10
-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3817/7/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:

Line 85: 
> The decrementing of ref cnt occurs in ReleaseQueryExecState, which will rem
Ah yes, my bad. I've made this a DCHECK


-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2550 Introduce query-wide execution context.
..


Patch Set 9:

(3 comments)

In this patch the following is addressed:
- DescriptorTbl is not shared anymore. I've moved it back to RuntimeState as we 
still need to change how the serialization/deserialization is done before we 
can make it shared. It will be done in a separate patch.

- QueryExecMgr now starts fragment threads so that refcount management stays in 
the scope of QueryExecMgr and is not passed to a lower level.

- FragmentInstanceExecState refcount mechanism removed. Now these objects have 
the same lifetime as QueryExecState and are destroyed when QueryExecState is 
destroyed.

- fragment_inst_exec_state_map_ is now indexed by fragment instance index and 
not fragment instance IDs. (Rebased on top of IMPALA-3988)

http://gerrit.cloudera.org:8080/#/c/3817/7/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:

Line 85: 
> how is this possible? it looks like an invariant that anything in the map w
No that's not how it works.
Everytime a new fragment comes in, FindOrCreateQueryExecState() is called (in 
RegisterFragmentInstanceWithQuery()) which increases the refcount by 1.
Everytime a fragment completes (in QEM->ExecInstance()), ReleaseQES() is called 
which decrements the refcount by 1.

This means that this refcount will ultimately reach 0.

Alternatively CancelPlanFragment() and PublishFilter() use QESGuard() which 
calls a GetQES() at the beginning of its scope and a ReleaseQES() when it goes 
out of scope which will effectively balance the refcount.


http://gerrit.cloudera.org:8080/#/c/3817/8/be/src/service/query-exec-mgr.cc
File be/src/service/query-exec-mgr.cc:

Line 54:   QueryExecState* query_exec_state = 
FindOrCreateQueryExecState(exec_params.query_ctx);
> comment seems redundant with the code (especially if you rename the method 
Done


PS8, Line 100: 
> FindOrCreateQueryExecState() given that it creates (as opposed to inserting
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#9).

Change subject: IMPALA-2550 Introduce query-wide execution context.
..

IMPALA-2550 Introduce query-wide execution context.

This patch is a header preview of a query wide execution state for
a backend.

Changes in design are as follows:

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryExecState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryExecState.
  This class is responsible for cleaning up the QueryExecState.

QueryExecState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FragmentExecStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryExecState.
  Every user of the QueryExecState must access it within the scope of
  ScopedQueryExecStateRef which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

QueryExecState (old) -> ClientRequestExecState:
  This is just a class name change.

  We still do not include shared state into the QueryExecState because
  there needs to be changes to DescriptorTbl, etc. before we can
  incorporate them into the QueryExecState. Will be done as separate
  patches.

P.S: This only shows what the changes would look like and as of yet
still does not compile.

Excludes:
 - Some class renames in places not important to the core patch logic.
 - Most .cc files to make use of changes made.

Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
---
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
R be/src/runtime/fragment-instance-exec-state.cc
R be/src/runtime/fragment-instance-exec-state.h
R be/src/runtime/fragment-instance-executor.cc
R be/src/runtime/fragment-instance-executor.h
A be/src/runtime/query-exec-state.cc
A be/src/runtime/query-exec-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/CMakeLists.txt
R be/src/service/client-request-exec-state.cc
R be/src/service/client-request-exec-state.h
D be/src/service/fragment-mgr.cc
D be/src/service/fragment-mgr.h
M be/src/service/impala-internal-service.h
A be/src/service/query-exec-mgr.cc
A be/src/service/query-exec-mgr.h
M common/thrift/ImpalaInternalService.thrift
20 files changed, 735 insertions(+), 360 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/9
-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has submitted this change and it was merged.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, 
overly verbose

The webserver address was always configured as 0.0.0.0 (meaning that
the webserver could be reached on any IP for that machine) unless
otherwise specified. This is not a correct value to dispay to the
user. This patch returns the hostname of the node, when requested,
if the webserver host address is 0.0.0.0.

This patch also does not print the coordinator link for very simple
queries, as it's not necessary and is unnecessarily verbose.

This patch also does away with pinging the impalad an extra time per
query for finding the host time and webserver address. It instead
remembers the webserver address at connect time and displays client
local time for every query instead.

Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Reviewed-on: http://gerrit.cloudera.org:8080/3994
Tested-by: Internal Jenkins
Reviewed-by: Sailesh Mukil <sail...@cloudera.com>
Tested-by: Sailesh Mukil <sail...@cloudera.com>
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/webserver.cc
M common/thrift/ImpalaService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
6 files changed, 69 insertions(+), 30 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Sailesh Mukil: Looks good to me, approved; Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


Patch Set 6: Verified+1

> Patch Set 6: Code-Review+2

Carrying +2 again.

This ran through GVM and got verified+1, but for some reason the CR+2 status 
got dropped. Since this passed GVM, submitting manually.

-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


Patch Set 6: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2550 Introduce query-wide execution context.
..


Patch Set 8:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/3817/7//COMMIT_MSG
Commit Message:

PS7, Line 17: passes this received fragment along to the Query
> which fragment?
the received fragment. I've clarified the comment.


PS7, Line 26:   Once scheduled for destruction, a thread in the QueryExecMgr 
will
:   destroy the QueryExecState.
> Why do we need a special thread for this?
I've explained this in the CR comment in the query-exec-mgr.h file.


PS7, Line 28:   Every user of the QueryExecState must access it within the 
scope of
:   ScopedQueryExecStateRef which guarantees ref count incrementing 
and
:   decrementing at entry/exit of the scope.
> It looks like we're implementing something that looks like a shared_ptr. I 
How I see it, reference counting is necessary because the lifetime of the QES 
is defined by how many fragments are around to use it.

If needed, I can quickly go over the overall design with you in person tomorrow 
and also talk about the shared_ptr vs refcount decision, so that it'll be a lot 
easier to review this patch.


http://gerrit.cloudera.org:8080/#/c/3817/7/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:

PS7, Line 54: and that the fragment instance must either run to completion
:   /// (with or withou
> or it can encounter an error?
Yes the fragment is executed on another thread. I've clarified that the 
fragment instance can encounter an error too.


PS7, Line 60: RegisterFragmentInstanceWithQuery
> How about just 'RegisterFragment'?
This is a fragment instance, so we would have to change it to 
RegisterFragmentInstance(). But that name is already used in 
QueryExecState::RegisterFragmentInstance(). So I added a 'WithQuery' here to 
curb confusion.

If you still think it's better to change the name, I'll go ahead and do that.


PS7, Line 68:   QueryExecState* GetQueryExecState(const TUniqueId& query_id);
: 
:   /// Decrements query_exec_state->num_current_references and 
schedules it for destruction
:   /// if the reference count hits zero.
:   void ReleaseQueryExecState(QueryExecState* query_exec_state);
> Who are the consumers of these APIs? I'm wondering if ref counting is reall
The consumers of these 2 functions are:
 - The fragment itself in QueryExecState::ExecInstance().
 - CancelPlanFragment() when a Cancel RPC is received.
 - PublishFilter() when a publish filter RPC is received.


PS7, Line 74: e que
> query's
Done


PS7, Line 74:  
> a
Done


PS7, Line 127:   /// The thread pool that is in charge of destroying a 
QueryExecState when it is no
 :   /// longer in use.
 :   ThreadPool<QueryExecState*> destroy_thread_;
> why does the QES destruction need to happen on a separate thread?
Currently I've made it that way because if a fragment is the last user of the 
QES, that fragment will decrease the refcount to 0, which means it should be 
deleted.
This happens in QueryExecState::ExecInstance().
If we delete it in the context of the same thread, it will be as if the 
QueryExecState is deleting itself which could cause memory corruption, etc.

Which is why the destruction of the QES is handed off to the QueryExecMgr on a 
separate thread.


-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#8).

Change subject: IMPALA-2550 Introduce query-wide execution context.
..

IMPALA-2550 Introduce query-wide execution context.

This patch is a header preview of a query wide execution state for
a backend.

Changes in design are as follows:

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryExecState if it is the first fragment to arrive for that
  query, and passes this received fragment along to the QueryExecState.
  This class is responsible for cleaning up the QueryExecState.

QueryExecState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FragmentExecStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryExecState.
  Every user of the QueryExecState must access it within the scope of
  ScopedQueryExecStateRef which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

QueryExecState (old) -> ClientRequestExecState:
  This is just a class name change.

P.S: This only shows what the changes would look like and as of yet
still does not compile.

Excludes:
 - Some class renames in places not important to the core patch logic.
 - Most .cc files to make use of changes made.

Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
---
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
R be/src/runtime/fragment-instance-exec-state.cc
R be/src/runtime/fragment-instance-exec-state.h
R be/src/runtime/fragment-instance-executor.cc
R be/src/runtime/fragment-instance-executor.h
A be/src/runtime/query-exec-state.cc
A be/src/runtime/query-exec-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/CMakeLists.txt
R be/src/service/client-request-exec-state.cc
R be/src/service/client-request-exec-state.h
D be/src/service/fragment-mgr.cc
D be/src/service/fragment-mgr.h
M be/src/service/impala-internal-service.h
A be/src/service/query-exec-mgr.cc
A be/src/service/query-exec-mgr.h
M common/thrift/ImpalaInternalService.thrift
20 files changed, 743 insertions(+), 374 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/8
-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


Patch Set 5: Code-Review+2

(1 comment)

Thanks for the review Henry.

Carry +2.

http://gerrit.cloudera.org:8080/#/c/3994/5/shell/impala_shell.py
File shell/impala_shell.py:

PS5, Line 874: if print_web_link:
 : print_web_link = (self.webserver_address != 
ImpalaShell.UNKNOWN_WEBSERVER)
> bit less confusing to write this as:
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Hello Henry Robinson,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/3994

to look at the new patch set (#6).

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..

IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, 
overly verbose

The webserver address was always configured as 0.0.0.0 (meaning that
the webserver could be reached on any IP for that machine) unless
otherwise specified. This is not a correct value to dispay to the
user. This patch returns the hostname of the node, when requested,
if the webserver host address is 0.0.0.0.

This patch also does not print the coordinator link for very simple
queries, as it's not necessary and is unnecessarily verbose.

This patch also does away with pinging the impalad an extra time per
query for finding the host time and webserver address. It instead
remembers the webserver address at connect time and displays client
local time for every query instead.

Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/webserver.cc
M common/thrift/ImpalaService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
6 files changed, 69 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/3994/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3986: Python hive-client may fail silently while dropping partitions

[Sailesh Mukil (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4083

to look at the new patch set (#2).

Change subject: IMPALA-3986: Python hive-client may fail silently while 
dropping partitions
..

IMPALA-3986: Python hive-client may fail silently while dropping partitions

It was noticed during a test run that a partition existed in the HMS
when it shouldn't have. The only possible explanation for that is that
the python based hive client failed to drop the partition when asked
to and we did not check for an error when it did so.

This patch just adds an assert to make sure that the partition is
dropped when instructed to, else it will cause the test to fail in
the right place.

Change-Id: I4241225d19c75a79179a61fef0c75c06a9a2ac39
---
M tests/common/impala_test_suite.py
1 file changed, 2 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/4083/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4241225d19c75a79179a61fef0c75c06a9a2ac39
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>

[Impala-ASF-CR] IMPALA-3986: Python hive-client may fail silently while dropping partitions

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3986: Python hive-client may fail silently while 
dropping partitions
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4083/1/tests/common/impala_test_suite.py
File tests/common/impala_test_suite.py:

PS1, Line 494: , 'Could
> If the fn returns True you should be able to drop this check
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4241225d19c75a79179a61fef0c75c06a9a2ac39
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3986: Python hive-client may fail silently while dropping partitions

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4083

Change subject: IMPALA-3986: Python hive-client may fail silently while 
dropping partitions
..

IMPALA-3986: Python hive-client may fail silently while dropping partitions

It was noticed during a test run that a partition existed in the HMS
when it shouldn't have. The only possible explanation for that is that
the python based hive client failed to drop the partition when asked
to and we did not check for an error when it did so.

This patch just adds an assert to make sure that the partition is
dropped when instructed to, else it will cause the test to fail in
the right place.

Change-Id: I4241225d19c75a79179a61fef0c75c06a9a2ac39
---
M tests/common/impala_test_suite.py
1 file changed, 2 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/4083/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4241225d19c75a79179a61fef0c75c06a9a2ac39
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#7).

Change subject: IMPALA-2550 Introduce query-wide execution context.
..

IMPALA-2550 Introduce query-wide execution context.

This patch is a header preview of a query wide execution state for
a backend.

Changes in design are as follows:

FragmentMgr -> QueryExecMgr
  The QueryExecMgr now receives incoming fragments, creates a new
  QueryExecState if it is the first fragment to arrive for that
  query, and passes this fragment along to the QueryExecState.
  This class is responsible for cleaning up the QueryExecState.

QueryExecState (new):
  This is the query wide state for the backend. It is initialized by
  the first fragment to arrive to the QueryExecMgr. This class is now
  responsible for creating FragmentExecStates and executing them.
  Its life is protected by a ref counting mechanism and it is
  scheduled for destruction once the ref count reaches zero.
  Once scheduled for destruction, a thread in the QueryExecMgr will
  destroy the QueryExecState.
  Every user of the QueryExecState must access it within the scope of
  ScopedQueryExecStateRef which guarantees ref count incrementing and
  decrementing at entry/exit of the scope.

QueryExecState (old) -> ClientRequestExecState:
  This is just a class name change.

P.S: This only shows what the changes would look like and as of yet
still does not compile.

Excludes:
 - Some class renames in places not important to the core patch logic.
 - Most .cc files to make use of changes made.

Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
---
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
R be/src/runtime/fragment-instance-exec-state.cc
R be/src/runtime/fragment-instance-exec-state.h
R be/src/runtime/fragment-instance-executor.cc
R be/src/runtime/fragment-instance-executor.h
A be/src/runtime/query-exec-state.cc
A be/src/runtime/query-exec-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/CMakeLists.txt
R be/src/service/client-request-exec-state.cc
R be/src/service/client-request-exec-state.h
D be/src/service/fragment-mgr.cc
D be/src/service/fragment-mgr.h
M be/src/service/impala-internal-service.h
A be/src/service/query-exec-mgr.cc
A be/src/service/query-exec-mgr.h
M common/thrift/ImpalaInternalService.thrift
20 files changed, 740 insertions(+), 374 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/3817/7
-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:

Line 101:   /// referenced as a shared_ptr to allow asynchronous calls to 
CancelPlanFragment()
> they are, and i have a patch out (3988) that embeds the instance index into
Yes, that will make it much simpler to extract the index.

However, I still think that a scenario such as the following could happen:
If there are 6 fragment instances for a query,

frag_inst#1 -> node1
frag_inst#6 -> node1

frag_inst#2 -> node2
frag_inst#3 -> node2

rest goes to node3

So the FragmentExecState vector in node1 will have a gap between index 1 and 
index 6.

Similarly gaps will exist for node2 and node 3.

I verified this by looking at some minicluster logs and the fragment instance 
IDs are not contiguous per backend.


-- 
To view, visit http://gerrit.cloudera.org:8080/3817
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I892091d6401acb2ea91ccb1623af54c6f9635e6c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: David Knupp <dkn...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3610: Account for memory used by filters in the coordinator

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3610: Account for memory used by filters in the 
coordinator
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4066/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS1, Line 1994: ++state->num_received;
  : DCHECK_LE(state->num_received, 
state->src_fragment_instance_idxs.size());
This doesn't seem very necessary anymore. It seems redundant seeing that we 
already have 'pending_count'.


-- 
To view, visit http://gerrit.cloudera.org:8080/4066
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c52c8a1c2e79ef370c77bf264885fc859678d1b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW: IMPALA-2550 Introduce query-wide execution context.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.
..


Patch Set 5:

(33 comments)

http://gerrit.cloudera.org:8080/#/c/3817/5//COMMIT_MSG
Commit Message:

PS5, Line 15: recieves
> receives
Done


PS5, Line 17:  
> A comma would be helpful here.
Done


PS5, Line 24: and is
> "and it is" ?
Done


PS5, Line 24: which
> This seems like an incomplete thought. Perhaps the "which" just needs to be
Done


http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:

Line 77:   TPlanFragmentInstanceCtx fragment_instance_ctx_;
> remove, unless there's something to do
Done


http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/fragment-exec-state.h
File be/src/runtime/fragment-exec-state.h:

Line 32: /// Execution state of a single plan fragment.
> what differentiates this from RuntimeState. ie, are these really two separa
The RuntimeState contains mostly the members populated at runtime like the 
QueryMemTracker, runtime profile counters, block managers, etc.

The FragmentExecState mostly has execution state associated with it and sets up 
the PlanFragmentExecutor.

If we were to merge them, that should be done as a separate patch as it would 
be quite a big change. However, I feel that would make the class pretty bloated.


PS5, Line 33: FragmentExecState
> Are we going to rename this to make it clear that it's instance-specific st
Yes, I've renamed it.


Line 40:   boost::mem_fn(::ReportStatusCb),
> lambda
Done


Line 42:   client_cache_(exec_env->impalad_client_cache()), 
exec_params_(params) {
> break into two lines, all other members are on separate lines
Done


Line 80:   // TODO: make pointer to shared per-query state
> what does this todo mean?
I forgot to remove that. Removed it now.


http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

Line 64: class PlanFragmentExecutor {
> needs new name. FragmentInstanceExecutor?
Done


http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/runtime/query-exec-state.h
File be/src/runtime/query-exec-state.h:

Line 55:   ObjectPool* obj_pool() const { return obj_pool_.get(); }
> .get()?
Oops, removed it.


Line 79:   QueryExecMgr* query_exec_mgr_;
> presumably "not owned"
Done


Line 85:   ObjectPool* obj_pool_;
> owner?
Done


Line 101:   /// referenced as a shared_ptr to allow asynchronous calls to 
CancelPlanFragment()
> why do we need a shared_ptr? we should get rid of implicit mem mgmt with sh
Having shared_ptrs are how it was done before. I've changed it to use raw 
pointers now though with an explicit mem mgmt model.

Are fragment instance numbers guaranteed to be consecutive in a backend? If 
not, we will have gaps in the vector/array.


Line 108:   std::shared_ptr GetFragmentExecState(
> remove shared_ptr or unique_ptr since we're now switching to an explicit me
Done


Line 115:   void FragmentInstanceExecThread(const TUniqueId& 
fragment_instance_id);
> why not just ExecInstance?
Done


http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/client-request-exec-state.h
File be/src/service/client-request-exec-state.h:

Line 44: /// Execution state of a query. This captures everything necessary
> clarify comment. this isn't just the "execution state".
Done


http://gerrit.cloudera.org:8080/#/c/3817/4/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:

PS4, Line 80: eId& fragment_instance_id);
> we can also encode the instance id as query id + instance offset/instance n
Yes I saw the patch for IMPALA-3988. This function has been removed in any case 
in the next patch set.


Line 91:   /// The caller should always check if obj->is_valid_ref() is 'true' 
before using it.
> please find a more concise name (maybe take inspiration from 'lock_guard'?)
Renamed it to QESGuard. Done.


http://gerrit.cloudera.org:8080/#/c/3817/5/be/src/service/query-exec-mgr.h
File be/src/service/query-exec-mgr.h:

Line 40: /// offer itself up for destruction when it is no longer being used 
and the
> 'no longer being used' is vague. mention (and make concrete in code) the re
Done


Line 59:   /// no-op (because the fragment is unregistered).
> you're also introducing terminology 'registered' and 'unregistered'. what a
The registered and unregistered semantics have already been in use. 
'Registered' means it has been successfully created and put in the map. 
'Unregistered' means that it cannot be found on the map any longer.

Now with the introduction of ref counts, it is the same as refcount>0 and 
refcount==0, because if it exists on the map, the refcount>0 and once the 
refcount==0, it's immediately removed from the map.


Line 60:   Status Regi

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#5).

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..

IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, 
overly verbose

The webserver address was always configured as 0.0.0.0 (meaning that
the webserver could be reached on any IP for that machine) unless
otherwise specified. This is not a correct value to dispay to the
user. This patch returns the hostname of the node, when requested,
if the webserver host address is 0.0.0.0.

This patch also does not print the coordinator link for very simple
queries, as it's not necessary and is unnecessarily verbose.

This patch also does away with pinging the impalad an extra time per
query for finding the host time and webserver address. It instead
remembers the webserver address at connect time and displays client
local time for every query instead.

Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/webserver.cc
M common/thrift/ImpalaService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
6 files changed, 70 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/3994/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


Patch Set 4:

(2 comments)

Also, all the shell tests pass with these changes.

http://gerrit.cloudera.org:8080/#/c/3994/2/be/src/util/webserver.cc
File be/src/util/webserver.cc:

PS2, Line 224: e).ok()) {
> GetHostname() makes no guarantees about what happens to the argument in the
Makes sense. Done.


http://gerrit.cloudera.org:8080/#/c/3994/2/shell/impala_shell.py
File shell/impala_shell.py:

PS2, Line 686: ult.version
> What's wrong with that? They're both the same address.
I was under the impression that some shell environments wouldn't create an 
automatic hyperlink for an address with "localhost". But changed it in any case 
as it doesn't matter that much.


-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#4).

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..

IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, 
overly verbose

The webserver address was always configured as 0.0.0.0 (meaning that
the webserver could be reached on any IP for that machine) unless
otherwise specified. This is not a correct value to dispay to the
user. This patch returns the hostname of the node, when requested,
if the webserver host address is 0.0.0.0.

This patch also does not print the coordinator link for very simple
queries, as it's not necessary and is unnecessarily verbose.

This patch also does away with pinging the impalad an extra time per
query for finding the host time and webserver address. It instead
remembers the webserver address at connect time and displays client
local time for every query instead.

Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/webserver.cc
M common/thrift/ImpalaService.thrift
M shell/impala_client.py
M shell/impala_shell.py
5 files changed, 30 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/3994/4
-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/3994/2/be/src/util/webserver.cc
File be/src/util/webserver.cc:

PS2, Line 223: sWildcardAddress(http_address_
> Can you use IsWildcardAddress(http_address_) ?
Done


PS2, Line 224: If GetHostname() returns an error, hostname will remain as
> um? if GetHostname() returns an error, hostname will be undefined.
I assign hostname in L222. So if GetHostname() returns an error, hostname will 
remain whatever value it was (which in this case is 0.0.0.0). Clarified the 
comment a bit.


http://gerrit.cloudera.org:8080/#/c/3994/2/shell/impala_client.py
File shell/impala_client.py:

PS2, Line 238: esult
> Just return result?
Done


http://gerrit.cloudera.org:8080/#/c/3994/2/shell/impala_shell.py
File shell/impala_shell.py:

PS2, Line 686: ult.version
> why?
Else it will print http://localhost:25000 vs http://127.0.0.1:25000
If you think that's fine, I can remove this line.


Line 877: 
> long line
Done


PS2, Line 880: (time.strftime("%Y-%m-%d %H:%M:%S", time.localtime()),
 : self.webserver_address))
> does this print for 'use' statements as well? I still think that's a bit ve
Yes it does. I forgot that we never used to print the time before. I've removed 
it.


-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, overly verbose

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#3).

Change subject: IMPALA-3893, IMPALA-3901: impala-shell prints incorrect 
coordinator address, overly verbose
..

IMPALA-3893, IMPALA-3901: impala-shell prints incorrect coordinator address, 
overly verbose

The webserver address was always configured as 0.0.0.0 (meaning that
the webserver could be reached on any IP for that machine) unless
otherwise specified. This is not a correct value to dispay to the
user. This patch returns the hostname of the node, when requested,
if the webserver host address is 0.0.0.0.

This patch also does not print the coordinator link for very simple
queries, as it's not necessary and is unnecessarily verbose.

This patch also does away with pinging the impalad an extra time per
query for finding the host time and webserver address. It instead
remembers the webserver address at connect time and displays client
local time for every query instead.

Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
---
M be/src/service/impala-beeswax-server.cc
M be/src/util/webserver.cc
M common/thrift/ImpalaService.thrift
M shell/impala_client.py
M shell/impala_shell.py
5 files changed, 31 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/3994/3
-- 
To view, visit http://gerrit.cloudera.org:8080/3994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9d167b66f2dd8629e40a7094d21ea7ce6b43d23b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   }
> Hmm, but what can a user do about that? and does an error mean the file isn
As we spoke, this metric is not to catch errors, but just to count the number 
of tries to open and close files.


http://gerrit.cloudera.org:8080/#/c/4018/5/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

PS5, Line 620: partition->tmp_hdfs_file = NULL;
Had to move this up too. Else, we could have cases where the metric is 
decremented twice for the same file.


-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Hello Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4018

to look at the new patch set (#5).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 15 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   ImpaladMetrics::NUM_FILES_OPEN_FOR_INSERT->Increment(-1);
> Should we do these before returning error? otherwise, the impalad metric ca
Yes, we would want that to not be decremented in case of an error.
So that test_verify_metrics can catch that a file(s) was not closed, in case of 
our test runs.

And otherwise too, users can see that there are some files still open.


-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

> Does this pass the tests?

Yes, I mentioned about that it passed the private build:
http://sandbox.jenkins.cloudera.com/job/impala-private-build-and-test/3984/

-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#4).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 13 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 3:

(1 comment)

Passed private build:
http://sandbox.jenkins.cloudera.com/job/impala-private-build-and-test/3984/

http://gerrit.cloudera.org:8080/#/c/4018/2/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 647: RETURN_IF_ERROR(FinalizePartitionFile(state, 
cur_partition->second.first));
> I still don't see why this is MergeStatus() instead of RETURN_IF_ERROR.  We
Right, that makes sense. I've made that change.


-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#3).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 14 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4018/1/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 648:   }
> MergeStatus() does this check for you.
I misunderstood the working of the ClosePartitionFile() function. So I removed 
the erase from the map now as it wouldn't save us too much.


Line 653: void HdfsTableSink::Close(RuntimeState* state) {
> why can't this stay in Close() so that it can more easily always happen, gi
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 16 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 1:

> > Uploaded patch set 1.
 > 
 > Contingent on success of the following private build:
 > http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/3969/

This failed some tests, so will fix and post another patch.

-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4018

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 22 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>