[Impala-ASF-CR] IMPALA-6655: Add owner information on database creation

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9637 )

Change subject: IMPALA-6655: Add owner information on database creation
..


Patch Set 1:

(2 comments)

Looks good to me as well, will merge after the final touches

http://gerrit.cloudera.org:8080/#/c/9637/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9637/1//COMMIT_MSG@13
PS1, Line 13: +-+--+-+
Not really related to your change, but our describe database looks a little 
strange. It's also quite different from Hive's output which looks nicer imo.

Let's not make any changes to the output in this patch. But can you file a JIRA 
to reconsider our output?


http://gerrit.cloudera.org:8080/#/c/9637/1/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java:

http://gerrit.cloudera.org:8080/#/c/9637/1/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java@111
PS1, Line 111: Preconditions.checkNotNull(owner_);
condense into a single line:

public String getOwner() { return Preconditions.checkNotNull(owner_); }



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id74ec9bd3cb7954999305e9cd9085cbf50921a78
Gerrit-Change-Number: 9637
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 16 Mar 2018 05:40:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6683: Fix infinite loop after restarting the catalog

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9684 )

Change subject: IMPALA-6683: Fix infinite loop after restarting the catalog
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2117/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
Gerrit-Change-Number: 9684
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 16 Mar 2018 04:37:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6683: Fix infinite loop after restarting the catalog

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9684 )

Change subject: IMPALA-6683: Fix infinite loop after restarting the catalog
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
Gerrit-Change-Number: 9684
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 16 Mar 2018 04:25:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..

IMPALA-6682: Remove MD5 assumption from pypi download script

pip_download.py assumes the python repository to use md5 as the hash
algorithm, which is not required by PEP-503 and not always true in
reality. This patch removes this assumption and enables support of all
hash algorithms in python hashlib.

Testing: buildall.sh works with 2 repos. One uses md5 and another uses
sha-256.

Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Reviewed-on: http://gerrit.cloudera.org:8080/9683
Reviewed-by: Tianyi Wang 
Reviewed-by: Taras Bobrovytsky 
Tested-by: Impala Public Jenkins
---
M infra/python/deps/pip_download.py
1 file changed, 30 insertions(+), 18 deletions(-)

Approvals:
  Tianyi Wang: Looks good to me, approved
  Taras Bobrovytsky: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 16 Mar 2018 03:39:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] [DOCS] Changed to --use local tz for unix timestamp conversions

[Alex Rodoni (Code Review)]

Alex Rodoni has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9620 )

Change subject: [DOCS] Changed to --use_local_tz_for_unix_timestamp_conversions
..


Patch Set 3:

John and Tim,
Could one of you give Code-Review +2 to get this move? Several '-'s really do 
not have to be in Gerrit for a week. Thank you!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id75ec73031b97aa8a3c61ccdbaea39db008b4093
Gerrit-Change-Number: 9620
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Anonymous Coward #381
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Mar 2018 03:10:07 +
Gerrit-HasComments: No

Re: [Impala-ASF-CR] IMPALA-6683: Fix infinite loop after restarting the catalog

[Dimitris Tsirogiannis]

Thanks for fixing this. I’ll take a look in about an  hour from now.

Dimitris

On Thu, Mar 15, 2018 at 6:49 PM Alex Behm (Code Review) 
wrote:

> Alex Behm *posted comments* on this change.
>
> View Change 
>
> Patch set 1:Code-Review +1
>
> Looks good to me. I think we should add a custom cluster test to cover
> this scenario, but not necessarily now.
>
>
> To view, visit change 9684 . To
> unsubscribe, visit settings .
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-MessageType: comment
> Gerrit-Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
> Gerrit-Change-Number: 9684
> Gerrit-PatchSet: 1
> Gerrit-Owner: Tianyi Wang 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dimitris Tsirogiannis 
> Gerrit-Reviewer: Tianyi Wang 
> Gerrit-Comment-Date: Fri, 16 Mar 2018 01:49:30 +
> Gerrit-HasComments: No
>

[Impala-ASF-CR] IMPALA-5695: restart first impalad() does not start impalad

[anujphadke (Code Review)]

anujphadke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9619 )

Change subject: IMPALA-5695: restart_first_impalad() does not start impalad
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py@80
PS1, Line 80: IMPALA_HOME = os.environ['IMPALA_HOME']
: IMPALAD_PATH = os.path.join(IMPALA_HOME,
:'bin/start-impalad.sh --server_name=server1')
> The issue is that impalad.restart() remains broken.  I think the proper fix
Thanks Adam and Freddy!
I agree. I looked into it and the issue does not happen if you just set 
shell=True in the popen in exec_process_async.
I am still digging up the code to root cause why this works. I will post my 
updates soon.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30a5c25e132deebb9731891364196af31e57d875
Gerrit-Change-Number: 9619
Gerrit-PatchSet: 1
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 16 Mar 2018 02:24:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5695: restart first impalad() does not start impalad

[Adam Holley (Code Review)]

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9619 )

Change subject: IMPALA-5695: restart_first_impalad() does not start impalad
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py@80
PS1, Line 80: IMPALA_HOME = os.environ['IMPALA_HOME']
: IMPALAD_PATH = os.path.join(IMPALA_HOME,
:'bin/start-impalad.sh --server_name=server1')
> The restart method in impala_cluster.py seems to be extracting this path fr
The issue is that impalad.restart() remains broken.  I think the proper fix is 
to modify the Process class in tests/common/impala_cluster.py to get and store 
the environment variables used for the process, and then update 
exec_process_async so that is can accept environment variables used in Popen.  
That way, any process we restart should have the same environment as opposed to 
just going off of the command line.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30a5c25e132deebb9731891364196af31e57d875
Gerrit-Change-Number: 9619
Gerrit-PatchSet: 1
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 16 Mar 2018 02:10:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9667 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I443889250efb3648bd27a845ca3078057af21729
Gerrit-Change-Number: 9667
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 16 Mar 2018 01:16:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6683: Fix infinite loop after restarting the catalog

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9684 )

Change subject: IMPALA-6683: Fix infinite loop after restarting the catalog
..


Patch Set 1: Code-Review+1

Looks good to me. I think we should add a custom cluster test to cover this 
scenario, but not necessarily now.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
Gerrit-Change-Number: 9684
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 16 Mar 2018 01:49:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9618 )

Change subject: IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations
..

IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations

Change-Id: I8991d85e77c7f5075525734145291457d50a7633
Reviewed-on: http://gerrit.cloudera.org:8080/9618
Reviewed-by: Thomas Tauber-Marshall 
Tested-by: Impala Public Jenkins
---
M docs/shared/impala_common.xml
1 file changed, 27 insertions(+), 5 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8991d85e77c7f5075525734145291457d50a7633
Gerrit-Change-Number: 9618
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Anonymous Coward #381
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9618 )

Change subject: IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations
..


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8991d85e77c7f5075525734145291457d50a7633
Gerrit-Change-Number: 9618
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Anonymous Coward #381
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 16 Mar 2018 01:48:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6683: Fix infinite loop after restarting the catalog

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9684


Change subject: IMPALA-6683: Fix infinite loop after restarting the catalog
..

IMPALA-6683: Fix infinite loop after restarting the catalog

Currently the catalog service ID topic item includes the ID string.
It causes the coexistence of multiple catalog service ID topic items
after the catalogd restarts. Impalad therefore keeps detecting the
change of catalog service ID and requests a full catalog update. This
patch uses "CATALOG_SERVICE_ID" as the topic item name instead.

With the patch impalad prints only one line of catalog change log after
the catalog restarts.

Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
---
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
1 file changed, 1 insertion(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1ee6c6477458e0f4dd31b12daa9ed5f146d84e7b
Gerrit-Change-Number: 9684
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-5717: Support for reading ORC data files

[Quanlong Huang (Code Review)]

Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9134 )

Change subject: IMPALA-5717: Support for reading ORC data files
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9134/5/testdata/bin/load_nested.py
File testdata/bin/load_nested.py:

http://gerrit.cloudera.org:8080/#/c/9134/5/testdata/bin/load_nested.py@297
PS5, Line 297: def load_orc():
> We have several tables in the functional-query dataset that use complex typ
That's really helpful! Thanks, Joe!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7b6ae4ce3b9ee8125b21993702faa87537790a4
Gerrit-Change-Number: 9134
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 16 Mar 2018 01:41:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6655: Add owner information on database creation

[Adam Holley (Code Review)]

Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9637 )

Change subject: IMPALA-6655: Add owner information on database creation
..


Patch Set 1: Code-Review+1

Don't see any issues, just make sure to pick up latest describe changes to 
check against.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id74ec9bd3cb7954999305e9cd9085cbf50921a78
Gerrit-Change-Number: 9637
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 16 Mar 2018 01:32:03 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9667 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..

Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

This reverts commit 0d95d3a03c3f82313169058950f9b6d926bad859.

Change-Id: I443889250efb3648bd27a845ca3078057af21729
Reviewed-on: http://gerrit.cloudera.org:8080/9667
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 67 insertions(+), 324 deletions(-)

Approvals:
  Alex Behm: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I443889250efb3648bd27a845ca3078057af21729
Gerrit-Change-Number: 9667
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..

IMPALA-6682: Remove MD5 assumption from pypi download script

pip_download.py assumes the python repository to use md5 as the hash
algorithm, which is not required by PEP-503 and not always true in
reality. This patch removes this assumption and enables support of all
hash algorithms in python hashlib.

Testing: buildall.sh works with 2 repos. One uses md5 and another uses
sha-256.

Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
---
M infra/python/deps/pip_download.py
1 file changed, 30 insertions(+), 18 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2112/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 16 Mar 2018 00:05:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..

IMPALA-6682: Remove MD5 assumption from pypi download script

pip_download.py assu0mes the python repository to use md5 as the hash
algorithm, which is not required by PEP-503 and not always true in
reality. This patch removes this assumption and enables support of all
hash algorithms in python hashlib.

Testing: buildall.sh works with 2 repos. One uses md5 and another uses
sha-256.

Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
---
M infra/python/deps/pip_download.py
1 file changed, 30 insertions(+), 18 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG@11
PS1, Line 11: nd enables
> Nit: "and enables support of all hash algorithms"
Done


http://gerrit.cloudera.org:8080/#/c/9683/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/9683/1/infra/python/deps/pip_download.py@44
PS1, Line 44: supported_algorithms = hashlib.algorithms_available
> We can check whether hashlib.algorithms_available is available in a try: bl
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:59:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:59:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9683/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/9683/1/infra/python/deps/pip_download.py@44
PS1, Line 44:   if algorithm not in ['md5', 'sha1', 'sha224', 'sha256', 
'sha384', 'sha512']:
We can check whether hashlib.algorithms_available is available in a try: block 
and fall back to the explicit list otherwise.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:50:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9683 )

Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9683/1//COMMIT_MSG@11
PS1, Line 11: nd support
Nit: "and enables support of all hash algorithms"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:50:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6682: Remove MD5 assumption from pypi download script

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9683


Change subject: IMPALA-6682: Remove MD5 assumption from pypi download script
..

IMPALA-6682: Remove MD5 assumption from pypi download script

pip_download.py assumes the python repository to use md5 as the hash
algorithm, which is not required by PEP-503 and not always true in
reality. This patch removes this assumption and support all hash
algorithms in python hashlib.

Testing: buildall.sh works with 2 repos. One uses md5 and another uses
sha-256.

Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
---
M infra/python/deps/pip_download.py
1 file changed, 25 insertions(+), 18 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie78f851490cbab10daa654aece36dab6e6c4329b
Gerrit-Change-Number: 9683
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 5: Code-Review+1

(2 comments)

I was able to follow the changes made here and they make sense to me.

http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@373
PS4, Line 373:   total_started = self._past_runners_num_queries_started
> I'd rather we not add a RLock at this point, since a lot of this script may
That makes sense.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@528
PS4, Line 528:   try:
> The relationship of query runners to queries is not clear and undocumented.
This code looks like it'll continue to spin them up at the given rate and only 
bounded by the total number of queries. Do we see a continuous increase in 
runners?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 23:32:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](impala-4835) IMPALA-6560: fix regression test for IMPALA-2376

[Tim Armstrong (Code Review)]

Tim Armstrong has abandoned this change. ( http://gerrit.cloudera.org:8080/9675 
)

Change subject: IMPALA-6560: fix regression test for IMPALA-2376
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: abandon
Gerrit-Change-Id: I024cb6b57647902b1735defb885cd095fd99738c
Gerrit-Change-Number: 9675
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](impala-4835) IMPALA-6587: free buffers before ScanRange::Cancel() returns

[Tim Armstrong (Code Review)]

Tim Armstrong has abandoned this change. ( http://gerrit.cloudera.org:8080/9674 
)

Change subject: IMPALA-6587: free buffers before ScanRange::Cancel() returns
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: abandon
Gerrit-Change-Id: I87182b6bd51b5fb0b923e7e4c8d08a44e7617db2
Gerrit-Change-Number: 9674
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9525 )

Change subject: IMPALA-6389: Make '\0' delimited text files work
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:

http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163:  tuple_delim_ || (tuple_delim_
> I agree the logic here is kind of broken; if tuple_delim_ == field_delim_,
Including DELIMITED_TUPLES as a conjunct here makes sense to me and also adding 
the dcheck to HasUnfinishedTuples().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 15 Mar 2018 22:36:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](impala-4835) IMPALA-6587: free buffers before ScanRange::Cancel() returns

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9674 )

Change subject: IMPALA-6587: free buffers before ScanRange::Cancel() returns
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9674/1/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9674/1/be/src/runtime/io/disk-io-mgr-test.cc@1197
PS1, Line 1197: ASSERT_OK(io_mgr.AllocateBuffersForRange(reader.get(), 
_client, range, MAX_BUFFER_SIZE));
Need to fix long line.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: comment
Gerrit-Change-Id: I87182b6bd51b5fb0b923e7e4c8d08a44e7617db2
Gerrit-Change-Number: 9674
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 22:31:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](impala-4835) IMPALA-6587: free buffers before ScanRange::Cancel() returns

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9674


Change subject: IMPALA-6587: free buffers before ScanRange::Cancel() returns
..

IMPALA-6587: free buffers before ScanRange::Cancel() returns

ScanRange::Cancel() now waits until an in-flight read finishes so
that the disk I/O buffer being processed by the disk thread is
freed when Cancel() returns.

The fix is to set a 'read_in_flight_' flag on the scan range
while the disk thread is doing the read. Cancel() blocks until
read_in_flight_ == false.

The code is refactored a little bit by moving some logic into
ScanRange, in order to do the change cleanly.

Testing:
Added query test that reproduces the issue.

Added a unit test and a stress option that reproduces the problem in a
targeted way.

Ran disk-io-mgr-stress test for a few hours. Ran it under TSAN and
inspected output to make sure there were no non-benign data races.

Change-Id: I87182b6bd51b5fb0b923e7e4c8d08a44e7617db2
---
M be/src/common/global-flags.cc
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M testdata/workloads/functional-query/queries/QueryTest/scanners.test
9 files changed, 183 insertions(+), 61 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: newchange
Gerrit-Change-Id: I87182b6bd51b5fb0b923e7e4c8d08a44e7617db2
Gerrit-Change-Number: 9674
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](impala-4835) IMPALA-6560: fix regression test for IMPALA-2376

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9675


Change subject: IMPALA-6560: fix regression test for IMPALA-2376
..

IMPALA-6560: fix regression test for IMPALA-2376

The test is modified to increase the size of collections allocated.
num_nodes and mt_dop query options are set to make execution as
deterministic as possible.

I looped the test overnight to try to flush out flakiness.

Adds support for row_regex lines in CATCH sections so that we can
match a larger part of the error message.

Change-Id: I024cb6b57647902b1735defb885cd095fd99738c
---
M testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch.test
M tests/common/impala_test_suite.py
2 files changed, 23 insertions(+), 11 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: newchange
Gerrit-Change-Id: I024cb6b57647902b1735defb885cd095fd99738c
Gerrit-Change-Number: 9675
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](impala-4835) IMPALA-4835: Part 3: switch I/O buffers to buffer pool

[Tim Armstrong (Code Review)]

Tim Armstrong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9673 )

Change subject: IMPALA-4835: Part 3: switch I/O buffers to buffer pool
..

IMPALA-4835: Part 3: switch I/O buffers to buffer pool

This is the final patch to switch the Disk I/O manager to allocate all
buffer from the buffer pool and to reserve the buffers required for
a query upfront.

* The planner reserves enough memory to run a single scanner per
  scan node.
* The multi-threaded scan node must increase reservation before
  spinning up more threads.
* The scanner implementations must be careful to stay within their
  assigned reservation.

The row-oriented scanners were most straightforward, since they only
have a single scan range active at a time. A single I/O buffer is
sufficient to scan the whole file but more I/O buffers can improve I/O
throughput.

Parquet is more complex because it issues a scan range per column and
the sizes of the columns on disk are not known during planning. To
deal with this, the reservation in the frontend is based on a
heuristic involving the file size and # columns. The Parquet scanner
can then divvy up reservation to columns based on the size of column
data on disk.

I adjusted how the 'mem_limit' is divided between buffer pool and non
buffer pool memory for low mem_limits to account for the increase in
buffer pool memory.

Testing:
* Added more planner tests to cover reservation calcs for scan node.
* Test scanners for all file formats with the reservation denial debug
  action, to test behaviour when the scanners hit reservation limits.
* Updated memory and buffer pool limits for tests.
* Added unit tests for dividing reservation between columns in parquet,
  since the algorithm is non-trivial.

Perf:
I ran TPC-H and targeted perf locally comparing with master. Both
showed small improvements of a few percent and no regressions of
note. Cluster perf tests showed no significant change.

Change-Id: I3ef471dc0746f0ab93b572c34024fc7343161f00
---
M be/src/exec/CMakeLists.txt
A be/src/exec/hdfs-parquet-scanner-test.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-column-readers.h
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-util.cc
M be/src/runtime/io/disk-io-mgr-stress-test.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-stress.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr.cc
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/util/BitUtil.java
M fe/src/test/java/org/apache/impala/util/BitUtilTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/disable-codegen.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M 
testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/codegen-mem-limit.test
M 
testdata/workloads/functional-query/queries/QueryTest/disk-spill-encryption.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test
M 

[Impala-ASF-CR](impala-4835) IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

[Tim Armstrong (Code Review)]

Tim Armstrong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9671 )

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
..

IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

In preparation for switching the I/O mgr to the buffer pool, this
removes and cleans up a lot of code so that the switchover patch starts
from a cleaner slate.

* Remove the free buffer cache (which will be replaced by buffer pool's
  own caching).
* Make memory limit exceeded error checking synchronous (in anticipation
  of having to propagate buffer pool errors synchronously).
* Simplify error propagation - remove the (ineffectual) code that
  enqueued BufferDescriptors containing error statuses.
* Document locking scheme better in a few places, make it part of the
  function signature when it seemed reasonable.
* Move ReturnBuffer() to ScanRange, because it is intrinsically
  connected with the lifecycle of a scan range.
* Separate external ReturnBuffer() and internal CleanUpBuffer()
  interfaces - previously callers of ReturnBuffer() were fudging
  the num_buffers_in_reader accounting to make the external interface work.
* Eliminate redundant state in ScanRange: 'eosr_returned_' and
  'is_cancelled_'.
* Clarify the logic around calling Close() for the last
  BufferDescriptor.
  -> There appeared to be an implicit assumption that buffers would be
 freed in the order they were returned from the scan range, so that
 the "eos" buffer was returned last. Instead just count the number
 of outstanding buffers to detect the last one.
  -> Touching the is_cancelled_ field without holding a lock was hard to
 reason about - violated locking rules and it was unclear that it
 was race-free.
* Remove DiskIoMgr::Read() to simplify the interface. It is trivial to
  inline at the callsites.

This will probably regress performance somewhat because of the cache
removal, so my plan is to merge it around the same time as switching
the I/O mgr to allocate from the buffer pool. I'm keeping the patches
separate to make reviewing easier.

Testing:
* Ran exhaustive tests
* Ran the disk-io-mgr-stress-test overnight

Change-Id: I722dd54c15708b17fd8b9816ea797350d217e88f
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/scanner-context.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/test-env.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
19 files changed, 582 insertions(+), 915 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: merged
Gerrit-Change-Id: I722dd54c15708b17fd8b9816ea797350d217e88f
Gerrit-Change-Number: 9671
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](impala-4835) IMPALA-4835: Part 2: Allocate scan range buffers upfront

[Tim Armstrong (Code Review)]

Tim Armstrong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9672 )

Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront
..

IMPALA-4835: Part 2: Allocate scan range buffers upfront

This change is a step towards reserving memory for buffers from the
buffer pool and constraining per-scanner memory requirements. This
change restructures the DiskIoMgr code so that each ScanRange operates
with a fixed set of buffers that are allocated upfront and recycled as
the I/O mgr works through the ScanRange.

One major change is that ScanRanges get blocked when a buffer is not
available and get unblocked when a client returns a buffer via
ReturnBuffer(). I was able to remove the logic to maintain the
blocked_ranges_ list by instead adding a separate set with all ranges
that are active.

There is also some miscellaneous cleanup included - e.g. reducing the
amount of code devoted to maintaining counters and metrics.

One tricky part of the existing code was the it called
IssueInitialRanges() with empty lists of files and depended on
DiskIoMgr::AddScanRanges() to not check for cancellation in that case.
See IMPALA-6564/IMPALA-6588. I changed the logic to not try to issue
ranges for empty lists of files.

I plan to merge this along with the actual buffer pool switch, but
separated it out to allow review of the DiskIoMgr changes separate from
other aspects of the buffer pool switchover.

Testing:
* Ran core and exhaustive tests.

Change-Id: Ic1fbb1ce2c886163a9ccecfed98f9ef5ee762e39
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/base-sequence-scanner.h
M be/src/exec/hdfs-lzo-text-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/scanner-context.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr-stress-test.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
25 files changed, 1,037 insertions(+), 599 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: impala-4835
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic1fbb1ce2c886163a9ccecfed98f9ef5ee762e39
Gerrit-Change-Number: 9672
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Hello Lars Volker, Michael Brown, David Knupp,

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

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

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

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..

IMPALA-6662: Make stress test resilient to hangs due to client crashes

The concurrent_select.py process starts multiple sub processes
(called query runners), to run the queries. It also starts 2 threads
called the query producer thread and the query consumer thread. The
query producer thread adds queries to a query queue and the query
consumer thread pulls off the queue and feeds the queries to the
query runners.

The query runner, once it gets queries, does the following:
...
  with _submit_query_lock:
increment(num_queries_started)
  run_query()# One runner crashes here.
  increment(num_queries_finished)
...

One of the runners crash inside run_query(), thereby never incrementing
num_queries_finished.

Another thread that's supposed to check for memory leaks
(but actually doesn't), periodically acquires '_submit_query_lock' and
waits for the number of running queries to reach 0 before releasing the
lock.

However, in the above case, the number of running queries will never
reach 0 because one of the query runners hasn't incremented
'num_queries_finished' and exited. Therefore, the poll_mem_usage()
function will hold the lock indefinitely, causing no new queries to be
submitted, nor the stress test to complete running.

This patch fixes the problem by changing the global trackers of
num_queries_started and num_queries_finished, etc. to a per
QueryRunner basis. Anytime we want to find the total number of queries
started/finished/cancelled, etc., we aggregate the values from all the
runners. We synchronize access by adding a new lock called the
_query_runners_lock.

In _wait_for_test_to_finish(), we periodically check if a QueryRunner has
died, and if it has, we make sure to update the num_queries_finished to
num_queries_started, since it may have died before updating the 'finished'
value, and we also count the error.

Also, reformatted small bits of the code and added more comments and debug
logs.

Testing: Ran the stress test with the new patch a few times to make sure
that it's doing what we expect.

TODO: Change the way we obtain query_idx. Also reformat other parts of code
so that it's easier to make future changes.

Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
---
M tests/stress/concurrent_select.py
1 file changed, 236 insertions(+), 81 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 4:

(6 comments)

> Patch Set 4:
>
> (6 comments)
>
> I had a first look and left some comments but I think the approach needs a 
> more thorough overhaul. It is not clear to me how queries and query-runners 
> relate to each other and the data structures involved in starting new runners 
> seem opaque to me. I think it would be easier to understand if we had a 
> proper multiprocessing.Queue() with work items (each representing a query) 
> and a pool of workers that consume items from the queue and put results into 
> a result queue, which then gets processed by the main process. Then the whole 
> bug would come down to an item being removed from the work queue but no 
> corresponding item showing up in the result queue. It would also simplify the 
> locking and computing sums of counters.
> 
> I can see how that could be out of scope for this change. Let me know if 
> you'd move forward with the current change as is and I'll do another pass.

I completely agree that we need to rethink the design of the stress test, 
however, I've only changed the parts of this patch that would help us unblock 
stress testing. There are still some parts of the code I'm not clear on, so 
making the larger change seems like a much bigger effort. It is something we 
definitely have to get back to at some point in the near future.

As for this patch, I'm just considering it a bug fix and not a major refactor.

http://gerrit.cloudera.org:8080/#/c/9635/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9635/4//COMMIT_MSG@53
PS4, Line 53: Testing: Ran the stress test with the new patch a few times to 
make sure
> You could add a pause to one of the runners and kill it to test this manual
I saw that query runners were dying in the middle (some of them keep dying for 
some reason and more spin up after that, I don't know why yet). I also saw that 
the same errors that used to cause the hang before did not cause the hang 
anymore.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@327
PS4, Line 327: # All values below are cumulative.
> maybe add a comment to describe how these are populated?
Done


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@373
PS4, Line 373:   for runner in self._query_runners:
> You could just call _total_num_queries_started() and _total_num_queries_fin
I'd rather we not add a RLock at this point, since a lot of this script may 
still be pretty buggy and adding a Recursive Lock would make it much harder for 
us to find them.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@429
PS4, Line 429: with self._query_runners_lock:
> I don't think holding the lock is necessary here. Python's GIL makes sure t
Done


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@528
PS4, Line 528: while self._total_num_queries_started() < 
self._num_queries_to_run:
> It is not clear to me whether a query runner will run a single query and th
The relationship of query runners to queries is not clear and undocumented. I 
did not make an attempt to refactor that as it would have ended up being a much 
larger change.

However, as I understand it, a query runner can run multiple queries (no clear 
limit on how many). But a lot of them fail randomly (don't know why yet), and 
as they keep failing, more keep getting spun up.

This is something I want to pick up at some point later on when I or someone 
else has more time.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@548
PS4, Line 548:   Process(target=self._start_single_runner, 
args=(query_runner, ))
> This looks hairy to me, can you add a comment explaining how exactly sharin
All the values that are shared between processes are between L973-L980. The 
rest are only process local variables.

There are still some in the StressRunner() class that are shared between 
processes, but they are involved in more complicated parts of the code, so I 
haven't touched them in this patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120:   return DoubleVal(trunc(
> pow(10.0, scale.val) is likely expensive to compute and it isn't clear that
Done. According to my benchmark, this implementation is much faster (over 10x) 
than the old one (for cases where scale is less than 20).


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120
PS1, Line 120:   return DoubleVal(trunc(
> Do we have a test that shows the better behavior for trunc() versus the pre
Yes, Alex, there is a test in expr-test.cc on line 5465


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121:   v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) 
/ pow(10.0, scale.val));
> Can you undo the code movement here so that the change is more clear?
Done


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121
PS1, Line 121:   v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) 
/ pow(10.0, scale.val));
> Should we also check for overflows here , set FunctionContext with an error
Anuj, the problem here is with double arithmetic. In general, it's not possible 
to represent a double number precisely. Which is why it is weird to do rounding 
and specify the number of digits to round to in decimal. I don't think that 
anything can be done about this.

This is exactly the reason why people should be using the decimal type.


http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:

http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80
PS1, Line 80:   static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, 
const BigIntVal&);
> Why the ordering change?  Also, why allow BigIntVal range here, isn't that
Fixed the ordering.

It makes sense to use bigintval because if someone passes in a smallint, it can 
be implicitly cast to bigint, but not the other way around. So bigint is kind 
of like an integer wildcard. Which overflow are you talking about?


http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266
PS1, Line 266:   [['sign'], 'DOUBLE', ['DOUBLE'], 
'impala::MathFunctions::Sign'],
> Should we also have a DECIMAL version?
Do you think we would need to have a sign() function for double, float, int, 
bigint, decimal, etc then?


http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280
PS1, Line 280:   [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 
'impala::MathFunctions::Ceil'],
> Don't we also need FLOAT versions of these if our goal is to return the sam
I agree, this should be addressed in a separate patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 15 Mar 2018 21:51:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9346 )

Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and 
related fns
..

IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns

Before this patch, the output type of round() ceil() floor() trunc() was
not always the same as the input type. It was also inconsistent in
general. For example, round(double) returned an integer, but
round(double, int) returned a double.

After looking at other database systems, we decided that the guideline
should be that the output type should be the same as the input type. In
this patch, we change the behavior of the previously mentioned functions
so that if a double is given then a double is returned.

We also modify the rounding behavior to always round away from zero.
Before, we were rounding towards positive infinity in some cases.

Testinging:
- Updated tests
- Ran an exhaustive build which passed.

Cherry-picks: not for 2.x

Change-Id: I77541678012edab70b182378b11ca8753be53f97
---
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
M be/src/exprs/math-functions.h
M be/src/exprs/scalar-fn-call.cc
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 107 insertions(+), 68 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97
Gerrit-Change-Number: 9346
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 4:

(6 comments)

I had a first look and left some comments but I think the approach needs a more 
thorough overhaul. It is not clear to me how queries and query-runners relate 
to each other and the data structures involved in starting new runners seem 
opaque to me. I think it would be easier to understand if we had a proper 
multiprocessing.Queue() with work items (each representing a query) and a pool 
of workers that consume items from the queue and put results into a result 
queue, which then gets processed by the main process. Then the whole bug would 
come down to an item being removed from the work queue but no corresponding 
item showing up in the result queue. It would also simplify the locking and 
computing sums of counters.

I can see how that could be out of scope for this change. Let me know if you'd 
move forward with the current change as is and I'll do another pass.

http://gerrit.cloudera.org:8080/#/c/9635/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9635/4//COMMIT_MSG@53
PS4, Line 53: Testing: Ran the stress test with the new patch a few times to 
make sure
You could add a pause to one of the runners and kill it to test this manually.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@327
PS4, Line 327: # All values below are cumulative.
maybe add a comment to describe how these are populated?

It think it would also help to add a brief comment to explain what each of them 
means.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@373
PS4, Line 373:   for runner in self._query_runners:
You could just call _total_num_queries_started() and 
_total_num_queries_finished() here. However, you'd need to switch 
_query_runners_lock to RLock.


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@429
PS4, Line 429: with self._query_runners_lock:
I don't think holding the lock is necessary here. Python's GIL makes sure that 
list operations themselves are threadsafe


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@528
PS4, Line 528: while self._total_num_queries_started() < 
self._num_queries_to_run:
It is not clear to me whether a query runner will run a single query and then 
exit, or whether it will keep running queries. Here it looks like we are 
starting new runners until we have run all of the queries, one per query. 
However, in _start_single_runner it looks like we are running a loop over the 
global query queue. Does this mean all runners continue to run queries, and we 
also continue to ramp up the number of runners all the time?


http://gerrit.cloudera.org:8080/#/c/9635/4/tests/stress/concurrent_select.py@548
PS4, Line 548:   Process(target=self._start_single_runner, 
args=(query_runner, ))
This looks hairy to me, can you add a comment explaining how exactly sharing of 
data works here? My understanding of Process() is that only particular members 
of query_runner (Value, Array) will use shared memory and be synchronized.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 21:16:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9618 )

Change subject: IMPALA-6654: [DOCS] Updated the Kudu/Sentry/Impala limitations
..


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2106/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8991d85e77c7f5075525734145291457d50a7633
Gerrit-Change-Number: 9618
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Anonymous Coward #381
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:38:40 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9667 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1: Code-Review+2

Ok, this one looks good.

We're reverting this on 2.x because the change includes a breaking change, so 
we only want it in 3.0 aka master.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I443889250efb3648bd27a845ca3078057af21729
Gerrit-Change-Number: 9667
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:37:33 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9667 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2105/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I443889250efb3648bd27a845ca3078057af21729
Gerrit-Change-Number: 9667
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:37:44 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Adam Holley (Code Review)]

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9667


Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..

Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

This reverts commit 0d95d3a03c3f82313169058950f9b6d926bad859.

Change-Id: I443889250efb3648bd27a845ca3078057af21729
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 67 insertions(+), 324 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I443889250efb3648bd27a845ca3078057af21729
Gerrit-Change-Number: 9667
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Alex Behm (Code Review)]

Alex Behm has abandoned this change. ( http://gerrit.cloudera.org:8080/9662 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Abandoned

Something went wrong here. Abandoning.
--
To view, visit http://gerrit.cloudera.org:8080/9662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: abandon
Gerrit-Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
Gerrit-Change-Number: 9662
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-6675: Default to --compact catalog topic=true.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9661 )

Change subject: IMPALA-6675: Default to --compact_catalog_topic=true.
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
Gerrit-Change-Number: 9661
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:18:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6675: Default to --compact catalog topic=true.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9661 )

Change subject: IMPALA-6675: Default to --compact_catalog_topic=true.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py
File tests/custom_cluster/test_compact_catalog_updates.py:

http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py@18
PS1, Line 18: true
> false
Done


http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py@28
PS1, Line 28: test_compact_catalog_topic_updates
> nit: test_non_compact_..
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
Gerrit-Change-Number: 9661
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:13:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6675: Default to --compact catalog topic=true.

[Alex Behm (Code Review)]

Hello Bharath Vissapragada,

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

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

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

Change subject: IMPALA-6675: Default to --compact_catalog_topic=true.
..

IMPALA-6675: Default to --compact_catalog_topic=true.

Testing:
- Ran a few queries locally
- Ran test_compact_catalog_updates.py locally
- Waiting for a perf/safety evaluation from Mostafa

Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
---
M be/src/common/global-flags.cc
M tests/custom_cluster/test_compact_catalog_updates.py
2 files changed, 6 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
Gerrit-Change-Number: 9661
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 

[Impala-ASF-CR] IMPALA-6338: Ensure successfully completed queries have full profile

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9656 )

Change subject: IMPALA-6338: Ensure successfully completed queries have full 
profile
..


Patch Set 1:

> There are some additional cases in ScannerContext and HdfsScanner
 > that check ScannerContext::is_cancelled(), which checks
 > RuntimeState::is_cancelled() but does not return CANCELLED_INTERNAL.
 > It looks like the data stream code may also have a similar pattern.

Good point. Fixing that will make this patch much messier, so I'm not really 
sure its the right approach anymore.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
Gerrit-Change-Number: 9656
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:10:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6338: Ensure successfully completed queries have full profile

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has abandoned this change. ( 
http://gerrit.cloudera.org:8080/9656 )

Change subject: IMPALA-6338: Ensure successfully completed queries have full 
profile
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
Gerrit-Change-Number: 9656
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9662 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1:

We're reverting this on 2.x because the change includes a breaking change, so 
we only want it in 3.0 aka master.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
Gerrit-Change-Number: 9662
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:10:03 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9662 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2104/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
Gerrit-Change-Number: 9662
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:07:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6672: test recover partitions.py should not be skipped on local FS

[Fredy Wijaya (Code Review)]

Fredy Wijaya has abandoned this change. ( http://gerrit.cloudera.org:8080/9646 )

Change subject: IMPALA-6672: test_recover_partitions.py should not be skipped 
on local FS
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4cbebd9e669b32399afcc15a2efa78235a803562
Gerrit-Change-Number: 9646
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Alex Behm 

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9662 )

Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
Gerrit-Change-Number: 9662
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Comment-Date: Thu, 15 Mar 2018 20:06:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6643: Add REFRESH METADATA fine-grained privilege

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#12). ( 
http://gerrit.cloudera.org:8080/9589 )

Change subject: IMPALA-6643: Add REFRESH METADATA fine-grained privilege
..

IMPALA-6643: Add REFRESH METADATA fine-grained privilege

Before this patch, ALL privilege is required to execute INVALIDATE
METADATA and having any privilege allows executing REFRESH  AND
INVALIDATE METADATA . With this patch, REFRESH METADATA
privilege is now required to execute INVALIDATE METADATA or REFRESH
statement.

These are the new GRANT/REVOKE statements introduced at server,
database, and table scopes.

GRANT REFRESH METADATA on SERVER svr TO ROLE testrole;
GRANT REFRESH METADATA on DATABASE db TO ROLE testrole;
GRANT REFRESH METADATA on TABLE db.tbl TO ROLE testrole;

REVOKE REFRESH METADATA on SERVER svr FROM ROLE testrole;
REVOKE REFRESH METADATA on DATABASE db FROM ROLE testrole;
REVOKE REFRESH METADATA on TABLE db.tbl FROM ROLE testrole;

TODO: Add end-to-end tests. See IMPALA-6674.

Testing:
- Ran front-end end-to-end tests

Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4
---
M common/thrift/CatalogObjects.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/Privilege.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/resources/authz-policy.ini.template
10 files changed, 182 insertions(+), 45 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4
Gerrit-Change-Number: 9589
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR](2.x) Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

[Adam Holley (Code Review)]

Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9662


Change subject: Revert "IMPALA-6479: Update DESCRIBE to respect column 
privileges"
..

Revert "IMPALA-6479: Update DESCRIBE to respect column privileges"

IMPALA-6479: Update DESCRIBE to respect column privileges.

Modified the Frontend to filter columns from the DESCRIBE
statement.  Additionally, if a user has select on at least
one column, they can run DESCRIBE and see most metadata.
If they do not have full table access, they will not see
location or view query metadata.

Testing:
Added tests to validate users that have one or more column
access can run describe and that the output is filtered
accordingly.

This reverts commit 0d95d3a03c3f82313169058950f9b6d926bad859.

Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
---
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/catalog/Column.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/DescribeResultFactory.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
13 files changed, 67 insertions(+), 324 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2020686ad06e1d2287e3f74473a2ec68572d532
Gerrit-Change-Number: 9662
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 19:50:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..

IMPALA-6488: removes use-after-free bug in lib_cache

Several recent runs have resulted in a boost mutex
invalid argument exception. The mutex in question is
the one that guards individual lib_cache entries
(LibCacheEntry::lock).

The exception is thrown due to the entry being deleted
by another thread prior to the current thread acquiring
the entry lock. This scenario happens when:
1) thread t1 looks up an existing entry e
   a. the lookup is guarded by a cache lock
   b. the cache lock is released (L356)
   c. e's lock is acquired on the next line and propagated
  up the call stack (look for the swaps)
   d. while e is locked, its use-count is incremented.
2) thread t2 deletes entry e
   a. the cache lock is acquired and e is looked up
   b. e's lock is acquired
   c. if e's usecount is 0 and was marked for removal, e is deleted.

If t2 runs following (1b), then t1 will acquire a lock on a deleted
entry (1c), causing the mutex exception.

There are two parts to the fix in this change:
(1) don't crash and (2) don't let concurrency regress.

1) remove 1b: keep the cache lock while acquiring e's lock.
The cache lock is then released and all other operations proceed as before.
Note that current lock ordering is still maintained (coarse to fine),
but now, the cache lock can be held while other threads block access
to the entry lock. When files have been copied, these operations
are short. While files are loading, accesses to the same entry
can serialize access to the entire cache.
2) add cache entry after its loaded.
Currently, on a cache miss, a new entry is created, locked,
added to the cache, and finally the cache is unlocked. The intent
is to allow other threads to concurrently load files for other entries.
However, now that the cache lock is held while the entry lock is held,
the loading thread will block other thread's from acquiring the same entry,
which will block access to the cache. The work-around is to release
the cache lock when loading a new entry and add the cache entry only when
its loaded. The workaround avoids expensive work while holding the
cache lock, but may do wasted work if multiple threads miss on the
same entry and load their entries in parallel.

Testing:
- ran core tests + hdfs
- added an end-to-end test that concurrently uses and drops/creates from
  the same lib-cache entry. with current settings, the use-after-free
  error is reliably reproduced.
- manual testing to examine concurrency of the following cases:
  - concurrent function creation from multiple lib files
(stresses coordinator)
  - concurrent function use from multiple lib files
(stresses backend)

Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Reviewed-on: http://gerrit.cloudera.org:8080/9626
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M tests/query_test/test_udfs.py
3 files changed, 219 insertions(+), 98 deletions(-)

Approvals:
  Dan Hecht: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

[Adam Holley (Code Review)]

Adam Holley has abandoned this change. ( http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Abandoned

Duplicate of 9514.
--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 

[Impala-ASF-CR] IMPALA-6675: Default to --compact catalog topic=true.

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9661 )

Change subject: IMPALA-6675: Default to --compact_catalog_topic=true.
..


Patch Set 1: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py
File tests/custom_cluster/test_compact_catalog_updates.py:

http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py@18
PS1, Line 18: true
false


http://gerrit.cloudera.org:8080/#/c/9661/1/tests/custom_cluster/test_compact_catalog_updates.py@28
PS1, Line 28: test_compact_catalog_topic_updates
nit: test_non_compact_..



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
Gerrit-Change-Number: 9661
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Thu, 15 Mar 2018 19:08:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5695: restart first impalad() does not start impalad

[Fredy Wijaya (Code Review)]

Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9619 )

Change subject: IMPALA-5695: restart_first_impalad() does not start impalad
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/9619/1/tests/authorization/test_grant_revoke.py@82
PS1, Line 82:'bin/start-impalad.sh --server_name=server1')
I think using os.path.join(IMPALA_PATH, 'bin', 'script-impalad.sh 
--server_name=server1') is better.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30a5c25e132deebb9731891364196af31e57d875
Gerrit-Change-Number: 9619
Gerrit-PatchSet: 1
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:56:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9525 )

Change subject: IMPALA-6389: Make '\0' delimited text files work
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h
File be/src/exec/delimited-text-parser.inline.h:

http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163
PS1, Line 163:  tuple_delim_ || (tuple_delim_
> I don't follow that. When DELIMITED_TUPLES is false, the comment says tuple
I agree the logic here is kind of broken; if tuple_delim_ == field_delim_, this 
isn't correct for !DELIMITED_TUPLES; however, in practice tuple_delim_ can not 
be equal to field_delim_ except in the unit test; this is prohibited by the 
frontend.

To compound the matter, the only place this matters, in 
hdfs-sequence-scanner.cc never checks this value, as by general design, the 
sequence scanner always knows where the tuple ends and never has to deal with 
unfinished tuples.

This makes me a bit loathe to make the logic here more complicated.

A good compromise might be to make this also include DELIMITED_TUPLES as a 
conjunct, and then add a DCHECK in HasUnfinishedTuples() that DELIMITED_TUPLES 
is true.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 1
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:34:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6389: Make '\0' delimited text files work

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9525 )

Change subject: IMPALA-6389: Make '\0' delimited text files work
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc
File be/src/exec/delimited-text-parser-test.cc:

http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc@160
PS2, Line 160:   TupleDelimitedTextParser nul_delim_parser(NUM_COLS, 0, 
is_materialized_col, NUL_DELIM);
> should we also test nul tuple delim with a column delim? if not, why is tha
Will add a test for this.


http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h
File be/src/exec/delimited-text-parser.h:

http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@22
PS2, Line 22: #include 
Will delete this


http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@41
PS2, Line 41: thme
> them
Done


http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@55
PS2, Line 55: char field_delim_ = '\0', char collection_item_delim = '^',
:   char escape_char = '\0');
> do these default argument values still have special meanings? can the delim
The only place these have interesting meanings is the unit test.  I don't think 
the defaults are very useful, but I also don't think removing them and adding a 
bunch of extra unexplained fields to the unit test is helpful either.


http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@171
PS2, Line 171:   bool IsFieldOrCollectionItemDelimiter(char c) {
> comment for this -- it's surprisingly non-trivial to understand what this i
Will add a comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8
Gerrit-Change-Number: 9525
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:17:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 4:

soft +1. I'd like to test it myself. I'll do that shortly.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 18:15:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6675: Default to --compact catalog topic=true.

[Alex Behm (Code Review)]

Alex Behm has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9661


Change subject: IMPALA-6675: Default to --compact_catalog_topic=true.
..

IMPALA-6675: Default to --compact_catalog_topic=true.

Testing:
- Ran a few queries locally
- Ran test_compact_catalog_updates.py locally
- Waiting for a perf/safety evaluation from Mostafa

Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
---
M be/src/common/global-flags.cc
M tests/custom_cluster/test_compact_catalog_updates.py
2 files changed, 4 insertions(+), 4 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I39a2dd42a21ef448b85278a8cef3c1d0112b844f
Gerrit-Change-Number: 9661
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Hello Michael Brown, David Knupp,

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

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

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

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..

IMPALA-6662: Make stress test resilient to hangs due to client crashes

The concurrent_select.py process starts multiple sub processes
(called query runners), to run the queries. It also starts 2 threads
called the query producer thread and the query consumer thread. The
query producer thread adds queries to a query queue and the query
consumer thread pulls off the queue and feeds the queries to the
query runners.

The query runner, once it gets queries, does the following:
...
  with _submit_query_lock:
increment(num_queries_started)
  run_query()# One runner crashes here.
  increment(num_queries_finished)
...

One of the runners crash inside run_query(), thereby never incrementing
num_queries_finished.

Another thread that's supposed to check for memory leaks
(but actually doesn't), periodically acquires '_submit_query_lock' and
waits for the number of running queries to reach 0 before releasing the
lock.

However, in the above case, the number of running queries will never
reach 0 because one of the query runners hasn't incremented
'num_queries_finished' and exited. Therefore, the poll_mem_usage()
function will hold the lock indefinitely, causing no new queries to be
submitted, nor the stress test to complete running.

This patch fixes the problem by changing the global trackers of
num_queries_started and num_queries_finished, etc. to a per
QueryRunner basis. Anytime we want to find the total number of queries
started/finished/cancelled, etc., we aggregate the values from all the
runners. We synchronize access by adding a new lock called the
_query_runners_lock.

In _wait_for_test_to_finish(), we periodically check if a QueryRunner has
died, and if it has, we make sure to update the num_queries_finished to
num_queries_started, since it may have died before updating the 'finished'
value, and we also count the error.

Also, reformatted small bits of the code and added more comments and debug
logs.

Testing: Ran the stress test with the new patch a few times to make sure
that it's doing what we expect.

TODO: Change the way we obtain query_idx. Also reformat other parts of code
so that it's easier to make future changes.

Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
---
M tests/stress/concurrent_select.py
1 file changed, 233 insertions(+), 81 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@850
PS2, Line 850:   # crashed impalads.
> Sorry. LOG events to go a very large debug log. I meant the console (stdout
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:45:07 +
Gerrit-HasComments: Yes

[native-toolchain-CR](cdh6.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9660 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..


Patch Set 1: Code-Review+2

Clean cherry-pick


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9660
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:43:10 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh6.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9660 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..

CDH-65655: use ssh instead of https for Kudu github

Some supported OSes (e.g. CentOS 6) don't support TLSv1.2 out of the
box. Github recently disallowed older versions of TLS, which broke
the Kudu build.

As an alternative, clone the Kudu github repo over SSH. A minor
advantage of this approach is that we can clone from any git repo and
don't depend on github's archive tarball feature.

Testing:
Built the toolchain on all supported OSes.

Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
---
M source/kudu/build.sh
1 file changed, 11 insertions(+), 18 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved; Verified

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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9660
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh6.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9659 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..

IMPALA-5717: Build ORC C++ lib in toolchain

This adds the C++ library from the Apache ORC project. 1.4.3 is the
latest release at the time of writing.

Includes two patches:
* One that allows building ORC against our versions of dependencies.
  ORC-266 fixes this upstream, but could not be cleanly cherry-picked.
* One that fixes a header that wasn't installed properly.

Testing:
Confirmed that it built locally, confirmed that the built utilities were
runnable after sourcing impala-config.sh.

Built on all supported distros.

Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
---
M buildall.sh
A source/orc/build.sh
A 
source/orc/orc-1.4.3-patches/0001-Allow-building-against-external-versions-of-dependen.patch
A 
source/orc/orc-1.4.3-patches/0002-ORC-239.-Install-missing-Statistics.hh-header-file.patch
4 files changed, 224 insertions(+), 0 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved; Verified

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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9659
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh6.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9660 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..


Patch Set 1: Verified+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9660
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:43:11 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh6.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9659 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..


Patch Set 1: Verified+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9659
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:43:00 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh6.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9659 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..


Patch Set 1: Code-Review+2

Clean cherry-pick


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9659
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:42:59 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh6.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9660


Change subject: CDH-65655: use ssh instead of https for Kudu github
..

CDH-65655: use ssh instead of https for Kudu github

Some supported OSes (e.g. CentOS 6) don't support TLSv1.2 out of the
box. Github recently disallowed older versions of TLS, which broke
the Kudu build.

As an alternative, clone the Kudu github repo over SSH. A minor
advantage of this approach is that we can clone from any git repo and
don't depend on github's archive tarball feature.

Testing:
Built the toolchain on all supported OSes.

Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
---
M source/kudu/build.sh
1 file changed, 11 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/60/9660/1
--
To view, visit http://gerrit.cloudera.org:8080/9660
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9660
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh6.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9659


Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..

IMPALA-5717: Build ORC C++ lib in toolchain

This adds the C++ library from the Apache ORC project. 1.4.3 is the
latest release at the time of writing.

Includes two patches:
* One that allows building ORC against our versions of dependencies.
  ORC-266 fixes this upstream, but could not be cleanly cherry-picked.
* One that fixes a header that wasn't installed properly.

Testing:
Confirmed that it built locally, confirmed that the built utilities were
runnable after sourcing impala-config.sh.

Built on all supported distros.

Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
---
M buildall.sh
A source/orc/build.sh
A 
source/orc/orc-1.4.3-patches/0001-Allow-building-against-external-versions-of-dependen.patch
A 
source/orc/orc-1.4.3-patches/0002-ORC-239.-Install-missing-Statistics.hh-header-file.patch
4 files changed, 224 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/59/9659/1
--
To view, visit http://gerrit.cloudera.org:8080/9659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh6.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9659
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh5.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9658 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..


Patch Set 1: Verified+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9658
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:39:17 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh5.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9658 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..

CDH-65655: use ssh instead of https for Kudu github

Some supported OSes (e.g. CentOS 6) don't support TLSv1.2 out of the
box. Github recently disallowed older versions of TLS, which broke
the Kudu build.

As an alternative, clone the Kudu github repo over SSH. A minor
advantage of this approach is that we can clone from any git repo and
don't depend on github's archive tarball feature.

Testing:
Built the toolchain on all supported OSes.

Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
---
M source/kudu/build.sh
1 file changed, 11 insertions(+), 18 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved; Verified

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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9658
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh5.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9658 )

Change subject: CDH-65655: use ssh instead of https for Kudu github
..


Patch Set 1: Code-Review+2

Clean cherry-pick


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9658
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:39:15 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh5.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9657 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..


Patch Set 1: Verified+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9657
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:38:33 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh5.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9657 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..

IMPALA-5717: Build ORC C++ lib in toolchain

This adds the C++ library from the Apache ORC project. 1.4.3 is the
latest release at the time of writing.

Includes two patches:
* One that allows building ORC against our versions of dependencies.
  ORC-266 fixes this upstream, but could not be cleanly cherry-picked.
* One that fixes a header that wasn't installed properly.

Testing:
Confirmed that it built locally, confirmed that the built utilities were
runnable after sourcing impala-config.sh.

Built on all supported distros.

Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
---
M buildall.sh
A source/orc/build.sh
A 
source/orc/orc-1.4.3-patches/0001-Allow-building-against-external-versions-of-dependen.patch
A 
source/orc/orc-1.4.3-patches/0002-ORC-239.-Install-missing-Statistics.hh-header-file.patch
4 files changed, 224 insertions(+), 0 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved; Verified

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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9657
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh5.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9657 )

Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..


Patch Set 1: Code-Review+2

Clean cherry-pick


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

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9657
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:38:27 +
Gerrit-HasComments: No

[native-toolchain-CR](cdh5.x) CDH-65655: use ssh instead of https for Kudu github

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9658


Change subject: CDH-65655: use ssh instead of https for Kudu github
..

CDH-65655: use ssh instead of https for Kudu github

Some supported OSes (e.g. CentOS 6) don't support TLSv1.2 out of the
box. Github recently disallowed older versions of TLS, which broke
the Kudu build.

As an alternative, clone the Kudu github repo over SSH. A minor
advantage of this approach is that we can clone from any git repo and
don't depend on github's archive tarball feature.

Testing:
Built the toolchain on all supported OSes.

Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
---
M source/kudu/build.sh
1 file changed, 11 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/58/9658/1
--
To view, visit http://gerrit.cloudera.org:8080/9658
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e2afd4ff936a26fb5669559fb61233a167b4269
Gerrit-Change-Number: 9658
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[native-toolchain-CR](cdh5.x) IMPALA-5717: Build ORC C++ lib in toolchain

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9657


Change subject: IMPALA-5717: Build ORC C++ lib in toolchain
..

IMPALA-5717: Build ORC C++ lib in toolchain

This adds the C++ library from the Apache ORC project. 1.4.3 is the
latest release at the time of writing.

Includes two patches:
* One that allows building ORC against our versions of dependencies.
  ORC-266 fixes this upstream, but could not be cleanly cherry-picked.
* One that fixes a header that wasn't installed properly.

Testing:
Confirmed that it built locally, confirmed that the built utilities were
runnable after sourcing impala-config.sh.

Built on all supported distros.

Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
---
M buildall.sh
A source/orc/build.sh
A 
source/orc/orc-1.4.3-patches/0001-Allow-building-against-external-versions-of-dependen.patch
A 
source/orc/orc-1.4.3-patches/0002-ORC-239.-Install-missing-Statistics.hh-header-file.patch
4 files changed, 224 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/native-toolchain 
refs/changes/57/9657/1
--
To view, visit http://gerrit.cloudera.org:8080/9657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: cdh5.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6abd86a5a683f19aa44b47629edf283b938b7e7e
Gerrit-Change-Number: 9657
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6338: Ensure successfully completed queries have full profile

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9656 )

Change subject: IMPALA-6338: Ensure successfully completed queries have full 
profile
..


Patch Set 1:

There are some additional cases in ScannerContext and HdfsScanner that check 
ScannerContext::is_cancelled(), which checks RuntimeState::is_cancelled() but 
does not return CANCELLED_INTERNAL. It looks like the data stream code may also 
have a similar pattern.

I haven't studied the code in detail, but it seems like those should be 
returning CANCELLED_INTERNAL in some cases.

We also return CANCELLED from DiskIoMgr, but that should never leak outside of 
the scan node. That should be cleaned up but I don't think affects the 
correctness of this patch.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
Gerrit-Change-Number: 9656
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:37:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6338: Ensure successfully completed queries have full profile

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9656 )

Change subject: IMPALA-6338: Ensure successfully completed queries have full 
profile
..


Patch Set 1:

Some context here - this issue was already fixed once:
https://gerrit.cloudera.org/#/c/8997/
which was reverted after a crash was seen in a build:
https://issues.apache.org/jira/browse/IMPALA-6484

I significantly re-wrote the patch to make things clearer and hopefully avoid 
these sorts of bugs cropping up in the future by being more explicit about the 
difference between 'cancelled manually or due to error', in which case we want 
to just tear down the query as quickly as possible, and 'cancelled due to all 
results having been returned', in which case we want to still be sure to put 
together a complete runtime profile


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
Gerrit-Change-Number: 9656
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:26:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@850
PS2, Line 850:   # crashed impalads.
> I print every runners exit code at L870. So if it does hit a SIGSEGV, it wo
Sorry. LOG events to go a very large debug log. I meant the console 
(stdout/stderr), like the status report that shows Done | Running | etc.


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@860
PS2, Line 860: increment(runner._num_queries_finished)
 : # Since we know that the runner crashed whi
> These values are on a per query runner basis (no '_total' prefix), so if 2
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@867
PS2, Line 867: # list.
> This is the same explanation as my previous comment, so it should be fine.
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@1079
PS2, Line 1079: if report.mem_limit_exceeded:
> I print the Query Runners PID in some other places, so the PID could serve
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:24:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6338: Ensure successfully completed queries have full profile

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9656


Change subject: IMPALA-6338: Ensure successfully completed queries have full 
profile
..

IMPALA-6338: Ensure successfully completed queries have full profile

test_profile_fragment_instances checks that, once all the results have
been returned, every fragment instance appears in the query profile
for a query that internally cancels fragment instances that are still
executing when the results have been fully returned.

Every fis is guaranteed to send a profile to the coordinator in
Finalize(), but previously fragment profiles were not applied by the
coordinator if the backend was 'done', defined as either all instances
having completed or one has entered an error state (including
cancelled).

So, the test could fail by the following sequence:
- Some fragment for a particular backend sends an update to the
  coordinator. 'returned_all_results_' is true, so the coordinator
  responds indicating the the backend should cancel its remaining
  fragments.
- Another fragment from that backend executes Finalize() and reports
  that it was cancelled. This causes the coordinator to consider the
  entire backend to be 'done'.
- A third fragment, which had not previously sent a report from the
  reporting thread, from the same backend executes Finalize(). This
  report will not be applied by the coordinator as the backend is
  considered 'done', so this fragment will not appear in the final
  profile.

The solution is to change the definition of 'done' to not include a
backend that has been cancelled due to all the results having been
returned but still has fragments that haven't completed.

THis patch accomplishes this by introducing a new Status,
CANCELLED_INTERNAL, which indicates a fragment that was cancelled due
to all of the results having been returned. A backend that has this
Status will continue to apply profile updates until all of its
fragments have completed or an error is encountered. For all other
purposes, CANCELLED_INTERNAL is equivalent to CANCELLED, i.e.
Status::IsCancelled() is true for both.

Testing:
- Ran test_profile_fragment_instances in a loop with no failures.
  I can reliably repro the original problem with a few carefully
  placed sleeps.

Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
---
M be/src/common/status.cc
M be/src/common/status.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.h
M common/thrift/generate_error_codes.py
M tests/query_test/test_observability.py
11 files changed, 60 insertions(+), 29 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I97b031548c64ac16d7e2a09b38baac2d30ac3340
Gerrit-Change-Number: 9656
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9635 )

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..


Patch Set 2:

(14 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py
File tests/stress/concurrent_select.py:

http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@a268
PS2, Line 268:
> Nit: Go ahead and leave extra blank line this in. flake8 prefers 2 spaces b
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@321
PS2, Line 321: acces
> Nit: access
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@339
PS2, Line 339: _record_runner_metrics_before_kill
> Can we have a more descriptive name? This happens after a process failed, b
_record_runner_metrics_before_evict() seems like a much better name. I changed 
it to that.


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@368
PS2, Line 368: @property
> Any reason you use the property decorator here but nowhere else?
As we spoke, we don't require @property for this, it was just moved from the 
base patchset. I removed it now.


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@376
PS2, Line 376: num_running = total_started - total_finished
> Maybe a nit, but can this exist outside the lock? That is, indented two spa
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@382
PS2, Line 382:   total_dequeued = self._past_runners_num_queries_dequeued
 :   for runner in self._query_runners:
 : total_dequeued += runner._num_queries_dequeued.value
 : return total_dequeued
> return self._total_num_queries_dequed_no_lock()
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@387
PS2, Line 387:   def _total_num_queries_dequeued_no_lock(self):
 : """ TODO: Get rid of this function after reformatting how we 
obtain query indices.
 : """
> I was confused about this function being dangerous until I saw that in its
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@647
PS2, Line 647: impalad
> Cool, this param isn't needed anymore since you bind the query_runner to an
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@850
PS2, Line 850: if runner.proc.exitcode != 0:
> While I believe queries that fail with error fail with some low positive in
I print every runners exit code at L870. So if it does hit a SIGSEGV, it would 
return -11 on a nix system. Is that what you were looking for?


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@860
PS2, Line 860: assert runner._num_queries_started.value - \
 : runner._num_queries_finished.value == 1
> I think this assert can be invalid if more than one runner has crashed. Let
These values are on a per query runner basis (no '_total' prefix), so if 2 
runners fail at the same time, each ones start minus finish has to at most be 1.


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@867
PS2, Line 867: assert runner._num_queries_started.value == 
runner._num_queries_finished.value
> Same with this.
This is the same explanation as my previous comment, so it should be fine.


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@877
PS2, Line 877:  == True:
> Remove. For Booleans, you can just use "if var". PEP-008 says to do it this
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@1078
PS2, Line 1078:   def _update_from_query_report
> Python nit: This is treated as a public method, so it shouldn't have the _
Done


http://gerrit.cloudera.org:8080/#/c/9635/2/tests/stress/concurrent_select.py@1079
PS2, Line 1079: LOG.debug("Updating runtime stats")
> Is there a simple way to distinguish this message across different QueryRun
I print the Query Runners PID in some other places, so the PID could serve as a 
unique ID for a runner. So I added that to the print statement now. Let me know 
if you think it should be something else.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:18:10 +

[Impala-ASF-CR] IMPALA-6662: Make stress test resilient to hangs due to client crashes

[Sailesh Mukil (Code Review)]

Hello Michael Brown, David Knupp,

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

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

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

Change subject: IMPALA-6662: Make stress test resilient to hangs due to client 
crashes
..

IMPALA-6662: Make stress test resilient to hangs due to client crashes

The concurrent_select.py process starts multiple sub processes
(called query runners), to run the queries. It also starts 2 threads
called the query producer thread and the query consumer thread. The
query producer thread adds queries to a query queue and the query
consumer thread pulls off the queue and feeds the queries to the
query runners.

The query runner, once it gets queries, does the following:
...
  with _submit_query_lock:
increment(num_queries_started)
  run_query()# One runner crashes here.
  increment(num_queries_finished)
...

One of the runners crash inside run_query(), thereby never incrementing
num_queries_finished.

Another thread that's supposed to check for memory leaks
(but actually doesn't), periodically acquires '_submit_query_lock' and
waits for the number of running queries to reach 0 before releasing the
lock.

However, in the above case, the number of running queries will never
reach 0 because one of the query runners hasn't incremented
'num_queries_finished' and exited. Therefore, the poll_mem_usage()
function will hold the lock indefinitely, causing no new queries to be
submitted, nor the stress test to complete running.

This patch fixes the problem by changing the global trackers of
num_queries_started and num_queries_finished, etc. to a per
QueryRunner basis. Anytime we want to find the total number of queries
started/finished/cancelled, etc., we aggregate the values from all the
runners. We synchronize access by adding a new lock called the
_query_runners_lock.

In _wait_for_test_to_finish(), we periodically check if a QueryRunner has
died, and if it has, we make sure to update the num_queries_finished to
num_queries_started, since it may have died before updating the 'finished'
value, and we also count the error.

Also, reformatted small bits of the code and added more comments and debug
logs.

Testing: Ran the stress test with the new patch a few times to make sure
that it's doing what we expect.

TODO: Change the way we obtain query_idx. Also reformat other parts of code
so that it's easier to make future changes.

Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
---
M tests/stress/concurrent_select.py
1 file changed, 233 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/9635/3
--
To view, visit http://gerrit.cloudera.org:8080/9635
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10c5dc9b8c2fffc471bac2279e348bc1d1fec3b7
Gerrit-Change-Number: 9635
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-5717: Support for reading ORC data files

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9134 )

Change subject: IMPALA-5717: Support for reading ORC data files
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9134/5/testdata/bin/load_nested.py
File testdata/bin/load_nested.py:

http://gerrit.cloudera.org:8080/#/c/9134/5/testdata/bin/load_nested.py@297
PS5, Line 297: def load_orc():
> I don't think this is the best way to load the data - I think we should add
We have several tables in the functional-query dataset that use complex types. 
I think it would make sense to extend those to do ORC.

For example, complextypes_fileformat is used for positive/negative testing of 
nested types for each file format. complextypes_multifileformat does something 
similar at a partition level. I think it would make sense for both of these to 
incorporate ORC. (See 
testdata/dataset/functional/functional_schema_template.sql)

complextypes_fileformat is used in our frontend planner tests that exercise the 
planner without the rest of execution. See 
fe/src/test/java/org/apache/impala/planner/PlannerTest.java. The specific test 
that uses complextypes_fileformat is testComplexTypesFileFormat(), which 
references 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test.
 This test is doing a similar check to what you are doing in 
nested-types-orc-basic.test, so I think it would make sense to incorporate that 
test into testComplexTypesFileFormat().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7b6ae4ce3b9ee8125b21993702faa87537790a4
Gerrit-Change-Number: 9134
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 17:03:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6643: Add REFRESH METADATA fine-grained privilege

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9589 )

Change subject: IMPALA-6643: Add REFRESH METADATA fine-grained privilege
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9589/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9589/8//COMMIT_MSG@15
PS8, Line 15: REFRESH METADATA
> WFM. We could also make it "METADATA REFRESH" privilege if we think users w
I'm sold. I now like "METADATA REFRESH" most :)

Fredy, Vuk, any thoughts?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4
Gerrit-Change-Number: 9589
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 16:23:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9420 )

Change subject: IMPALA-3866: Improve error reporting for scratch write errors
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44:  {ENXIO,   "Device doesn't exist."}});
> The request was to enrich the basic error messages from GetStrErrMsg() so t
Okay. I thought GetStrErrMsg() gives similar text, but maybe not.
Is the motivation for this documented somewhere? If no, how about putting a 
quick comment explaining why we have this rather than just using GetStrErrMsg() 
since I don't think that's obvious.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 16:09:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2103/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:54:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..


Patch Set 6: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:54:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:53:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6643: Add REFRESH METADATA fine-grained privilege

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9589 )

Change subject: IMPALA-6643: Add REFRESH METADATA fine-grained privilege
..


Patch Set 11:

Before I forget, I was expecting to see tests for grant, drop, and show so that 
the feature can be fully demonstrated. If testing show + auth is difficult, pls 
add a jira and a todo in the most relevant place where such a test would show 
up.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4
Gerrit-Change-Number: 9589
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:53:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6488: removes use-after-free bug in lib cache

[David Knupp (Code Review)]

David Knupp has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9626 )

Change subject: IMPALA-6488: removes use-after-free bug in lib_cache
..


Patch Set 5: Code-Review+1

Test looks fine to me. Someone else should +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f178a2114cb3969dbb920949ddff4e8220b742e
Gerrit-Change-Number: 9626
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:52:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6643: Add REFRESH METADATA fine-grained privilege

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9589 )

Change subject: IMPALA-6643: Add REFRESH METADATA fine-grained privilege
..


Patch Set 11: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c3c5a51fe493d39fd719c7a388d4d5760049ce4
Gerrit-Change-Number: 9589
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 15 Mar 2018 15:41:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors

[Gabor Kaszab (Code Review)]

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9420 )

Change subject: IMPALA-3866: Improve error reporting for scratch write errors
..


Patch Set 13:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19
PS12, Line 19: IMPALA_RUNTIME_IO_ERROR_CONVERTER_H
> needs update
Thx, I don't know how I could have missed this.
Done


http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@41
PS12, Line 41:   /// text is provided by the errno_to_error_text_map_ member. 
The key-value pairs in
> it'd be good to comment on 'param's in the public interface (like you do be
Good point. I'll move the comment from the private method here as I don't see 
the point of having it for both functions.
Done


http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44:  {ENXIO,   "Device doesn't exist."}});
> out of curiosity, why do we define these rather than using GetStrErrMsg() a
The request was to enrich the basic error messages from GetStrErrMsg() so that 
support can more easily identify the underlying issue for a scratch write error.
These custom error messages are the result of the consultation with Jeszy and 
Tim.


http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@61
PS12, Line 61: GetStrErrMsg
> GetStrErrMsg() has a comment about saving errno early or else it might get
Good point. Made a second version of GetStrErrMsg() that receives the error 
code to make sure it's not overriden.


http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h
File be/src/runtime/io/local-file-system.h:

http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h@33
PS12, Line 33:   // A wrapper around open() and fdopen(). For the possible 
values of oflag and mode
> I think this needs a comment explaining option1 and option2. Maybe it would
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 15 Mar 2018 13:19:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors

[Gabor Kaszab (Code Review)]

Hello Attila Jeges, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3866: Improve error reporting for scratch write errors
..

IMPALA-3866: Improve error reporting for scratch write errors

The error messages coming from DiskIoMgr::Write() are enhanced by this
change. A mapping is introduced between the errno set by open(),
fdopen(), fseek(), fwrite() or fclose() low level functions and an
error message for displaying purposes. If any of these functions
fail then the returned error message is taken from this mapping.

In addition there were two functions, NewFile() and
FileAllocateSpace() that always returned Status::OK(). I made them
void and removed the status checks from the call sites.

For testing purposes a fault injection mechanism is introduced to
simulate the cases when the above mentioned functions fail.

Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
---
M be/src/runtime/io/CMakeLists.txt
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
A be/src/runtime/io/error-converter.cc
A be/src/runtime/io/error-converter.h
A be/src/runtime/io/local-file-system-with-fault-injection.cc
A be/src/runtime/io/local-file-system-with-fault-injection.h
A be/src/runtime/io/local-file-system.cc
A be/src/runtime/io/local-file-system.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/error-util.cc
M be/src/util/error-util.h
17 files changed, 621 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/13
--
To view, visit http://gerrit.cloudera.org:8080/9420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Tim Armstrong