[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder

[Pranay Singh (Code Review)]

Hello Taras Bobrovytsky, Joe McDonnell, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
..

IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder

Currently DictDecoder class and DictEncoder class uses std::vector
to store the tables mapping codeword to value and vice-versa. It is
hard to detect the memory usage by these tables when they becomes
very large, since this memory is not accounted by Impala's memory
mangement infrastructure.

This patch uses the memory tracker of HdfsScanner to track the memory used
by dictionary in DictDecoder class. Similary it uses memory tracker of
HdfsTableSink to track the memory used by dictionary in DictEncoder class.

Memory for the dictionary, stored as std::vector is still allocated
from std:allocator but the amount allocated is accounted by
introducing a counter which is incremented and decremented as the
memory is consumed and released by vector.

Testing
---
Ran all the backend and end-end tests with no failures.

Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
---
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-column-readers.h
M be/src/util/dict-encoding.h
M be/src/util/dict-test.cc
5 files changed, 173 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8034/19
--
To view, visit http://gerrit.cloudera.org:8080/8034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Number: 8034
Gerrit-PatchSet: 19
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4927: Impala should be able to handle invalid input from Sentry

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8588


Change subject: IMPALA-4927: Impala should be able to handle invalid input from 
Sentry
..

IMPALA-4927: Impala should be able to handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue interface
the code was instrumented to have a invalid Role " " and see how the condition
is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 29 insertions(+), 24 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 31 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4927: Impala should be able to handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada,

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

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

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

Change subject: IMPALA-4927: Impala should be able to handle invalid input from 
Sentry
..

IMPALA-4927: Impala should be able to handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 31 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@144
PS5, Line 144: sentryPrivlist =
 :   
sentryPolicyService_.listRolePrivileges(processUser_, role.getName());
> indentation is off
Corrected the identation.


http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: AuthorizationException
> An AuthorizationException is thrown when the sentry client throws a SentryA
Changed the code to handle all exceptions



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:40:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 7
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have an invalid Role " ",
and SHOW ROLES statement was executed from impala shell to see
how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@148
PS5, Line 148:   LOG.error("Error listing the Role name:" + 
roleName, e);
> Would be nice to have a space after the ':'
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:47:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/5/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS5, Line 146: Exception e) {
> I feel we should catch the specific set of exceptions documented for the AP
I agree with Zach's thought that we won't be changing from the current behavior 
of this code except for the case when an AuthorizationException is encountered.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 21:25:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/9/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS9, Line 146: } catch (AuthorizationException e) {
> The original issue that prompted this JIRA saw listRolePrivileges() throwin
Ok so changing AuthorizationException to ImpalaException



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 22 Nov 2017 00:24:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Dimitris Tsirogiannis, Zach 
Amsden, Alex Behm,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have an invalid Role " ",
and SHOW ROLES statement was executed from impala shell to see
how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Introduced new error message when scanning a corrupt Sequence or RCFile.
Added new checks to detect buffer overrun while handling Sequence or RCFile.

Testing:
  a) Made changes to fuzz test for RCFile/Sequence file, ran fuzz test in a loop
  with 200 iteration without failure.

  b) Ran exhaustive test on the changes without failure.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 259 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/8936/19
--
To view, visit http://gerrit.cloudera.org:8080/8936
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 19
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 18:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8936/18/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8936/18/be/src/runtime/timestamp-parse-util.cc@370
PS18, Line 370:   DCHECK_GT(len, 0);
> Please remove this, it's not valid. E.g. cast('' as timestamp).
Sure was an oversight should have removed it, thanks for pointing out.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 18
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 08 May 2018 22:31:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Introduced new error message when scanning a corrupt Sequence or RCFile.
Added new checks to detect buffer overrun while handling Sequence or RCFile.

Testing:
  a) Made changes to fuzz test for RCFile/Sequence file, ran fuzz test in a loop
  with 200 iteration without failure.

  b) Ran exhaustive test on the changes without failure.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
10 files changed, 260 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/8936/18
--
To view, visit http://gerrit.cloudera.org:8080/8936
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 18
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10450 )

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..


Patch Set 2:

> > (2 comments)
 > >
 > > Just interested in this optimization. May I ask some questions?
 > >
 > > Looks like we are optimizing the case when partitionsToUpdate !=
 > > null and partitions were neither dropped, created. Can we
 > optimize
 > > the case that partitionsToUpdate != null and some partitions are
 > > dropped? For example when an INSERT OVERWRITE statement updates
 > the
 > > majority of the partitions and only drops few of them.
 >
 > Will this case not introduce inconsistency between HMS and Impala?

The problem with optimization is that introduces inconsistency, something which 
my change introduces too, when ALTER TABLE is done and HMS crashes.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Sat, 19 May 2018 18:18:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10450 )

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..


Patch Set 2:

> (2 comments)
 >
 > Just interested in this optimization. May I ask some questions?
 >
 > Looks like we are optimizing the case when partitionsToUpdate !=
 > null and partitions were neither dropped, created. Can we optimize
 > the case that partitionsToUpdate != null and some partitions are
 > dropped? For example when an INSERT OVERWRITE statement updates the
 > majority of the partitions and only drops few of them.

Will this case not introduce inconsistency between HMS and Impala?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Sat, 19 May 2018 18:15:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10450 )

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..


Patch Set 2:

(2 comments)

> (2 comments)
 >
 > Just interested in this optimization. May I ask some questions?
 >
 > Looks like we are optimizing the case when partitionsToUpdate !=
 > null and partitions were neither dropped, created. Can we optimize
 > the case that partitionsToUpdate != null and some partitions are
 > dropped? For example when an INSERT OVERWRITE statement updates the
 > majority of the partitions and only drops few of them.

Will this case not cause introduce inconsistency between HMS and Impala ?

http://gerrit.cloudera.org:8080/#/c/10450/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/10450/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1409
PS2, Line 1409: size() == 0
> nit: can be simplified by isEmpty()
OK


http://gerrit.cloudera.org:8080/#/c/10450/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1410
PS2, Line 1410: partitionsToUpdateFileMdByPath = 
getPartitionsByPath(partitionsToUpdate);
  :   loadMetadataAndDiskIds(partitionsToUpdateFileMdByPath, 
true);
> Looks like the original codes perform the same as these two lines. Since dr
The new change behaves very much like the old code except for the case when 
dirtyPartitions exist in  that case there is an overhead of dropping and 
loading the dirty partitions from Metastore.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Sat, 19 May 2018 18:14:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10450 )

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10450/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/10450/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1410
PS2, Line 1410: partitionsToUpdateFileMdByPath = 
getPartitionsByPath(partitionsToUpdate);
  :   loadMetadataAndDiskIds(partitionsToUpdateFileMdByPath, 
true);
> I don't think so. In line 1404, the dirtyPartitions are all added to partit
Yes missed on that one, I'll rollback this change.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Mon, 21 May 2018 15:53:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has abandoned this change. ( http://gerrit.cloudera.org:8080/10450 
)

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10450


Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations.
..

IMPALA-6994: Avoid reloading a table's HMS data for file-only operations.

Catalog currently loads the HMS data even, if only the file metadata load
is requested. This change detects some of those updates and tries to skip
the loading of HMS data if only the file metadata load is requested.

Testing:
  Ran the core test without failures.

Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
1 file changed, 39 insertions(+), 23 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaabdf38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 10450
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6994:Avoid reloading a table's HMS data for file-only operations

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10587


Change subject: IMPALA-6994:Avoid reloading a table's HMS data for file-only 
operations
..

IMPALA-6994:Avoid reloading a table's HMS data for file-only operations

  The problem is that while inserting a  new row to an unpartitioned HDFS table,
  or to an existing partition, the catalogd makes unecessary call to Hive Meta
  Store to do a getTable() and list the partitions of the table, when in fact
  only the file metadata for HDFS tables needs to be updated.

  This extra call to HMS introduces a point of failure, we need to handle
  for error scenario when Hive MetaStore crashes. This change removes the
  extra call to Hive Meta Store for the case when a row is inserted to an
  existing partition in HDFS table or when a row is added to unpartitioned
  table. Thus, an optimization as it reduces the call to Hive Meta Store
  during Update of Catalog.

  Testing: Ran core test without failure.

Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
2 files changed, 34 insertions(+), 17 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
Gerrit-Change-Number: 10587
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10847


Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

  Currently Impalad does not have signal handlers, so it's  hard to triage
  issues related to impala crash as to what caused them. This change adds
  signal handlers that logs the name of the signals encountered.

  Testing:
Used kill to send signals to impalad
kill -s SIGTERM 
kill -s SIGQUIT 
kill -s SIGSEGV 
kill -s SIGABRT 

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/service/impalad-main.cc
1 file changed, 34 insertions(+), 0 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 1:

> How does your change interact with the Breakpad integration?
 > SIGSEGV and SIGABRT are also handled by the Breakpad signal handler
 > (https://chromium.googlesource.com/breakpad/breakpad/+/master/src/client/linux/handler/exception_handler.cc#115).
 >
 > It should have a test, possibly in the custom-cluster-tests folder,
 > to validate that Impala does the expected for each signal it could
 > receive.
 >
 > An alternative approach could be to print the received signal into
 > the logs when writing a minidump.

I'm resorting to default handlers after the message has been dumped but anyways 
I'll test it with breakpad and see if it affects the core/minidump creation


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Mon, 02 Jul 2018 16:54:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 15:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@91
PS15, Line 91: excceeded
> typo: exceeded
Done


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@545
PS15, Line 545: if (max_tuples < 0) {
> It seems like this is catching the problem too late. How does it get into t
Done


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc@370
PS15, Line 370:   if (UNLIKELY(str == NULL || len <= 0)) {
> We're already checking len here but the problem is that we're trimming whit
Introduced a DCHECK_GT(len, 0) and a check in the caller.


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc@477
PS15, Line 477: if (lazy_len > 0 && ParseFormatTokensByStr(_ctx)) {
> Looks like you hit upon a different bug here - I can trigger that DCHECK wi
I have removed the check



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 15
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 03 May 2018 18:44:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Introduced new error message when scanning a corrupt Sequence or RCFile.
Added new checks to detect buffer overrun while handling Sequence or RCFile.

Testing:
  a) Made changes to fuzz test for RCFile/Sequence file, ran fuzz test in a loop
  with 200 iteration without failure.

  b) Ran exhaustive test on the changes without failure.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
10 files changed, 259 insertions(+), 73 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/36/8936/16
--
To view, visit http://gerrit.cloudera.org:8080/8936
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 16
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Introduced new error message when scanning a corrupt Sequence or RCFile.
Added new checks to detect buffer overrun while handling Sequence or RCFile.

Testing:
  a) Made changes to fuzz test for RCFile/Sequence file, ran fuzz test in a loop
  with 200 iteration without failure.

  b) Ran exhaustive test on the changes without failure.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/runtime/timestamp-parse-util.cc
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
10 files changed, 258 insertions(+), 72 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 17
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 16:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@588
PS15, Line 588: }
> I think we should catch the invalid column metadata when we read it in GetC
Since the end of column is calculated here I am checking it here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 16
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 03 May 2018 18:55:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8949


Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..

IMPALA-6075: Add Impala daemon metric for catalog version.

This patch adds a new metric that shows the current version of catalog
which is currently used by impala daemon.

Testing:
Verified manually that the new metric for catalog version is displayed and it
corresponds to the latest version in catalogd.INFO log file.

Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
---
M be/src/service/impala-server.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
4 files changed, 21 insertions(+), 0 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 2:

> Can we test all the cases fixed this with 100% reproducible cases?
 > For Ex:
 > I commented out the change in  decompress.cc and ran the fuzz test
 > and it passed.

 > Can we test all the cases fixed this with 100% reproducible cases?
 > For Ex:
 > I commented out the change in  decompress.cc and ran the fuzz test
 > and it passed.

If you run a couple of times without the fix the assertion will be hit. I'll 
see if I can change the test so that we hit the assert every time


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 08 Jan 2018 16:59:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8949 )

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8949/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8949/1/be/src/service/impala-server.cc@1061
PS1, Line 1061: 
ImpaladMetrics::CATALOG_CURR_VER->set_value(catalog_update_info_.catalog_version);
> Are you sure you don't need the catalog_version_lock_ to access this here?
Changed the logic that takes the catalog_version_lock_ to update the newly 
added metric


http://gerrit.cloudera.org:8080/#/c/8949/1/be/src/service/impala-server.cc@1514
PS1, Line 1514:   
ImpaladMetrics::CATALOG_CURR_VER->set_value(min_req_catalog_version);
> I think this may not be necessary?
Removed it.


http://gerrit.cloudera.org:8080/#/c/8949/1/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/8949/1/be/src/util/impalad-metrics.h@113
PS1, Line 113: CATALOG_CURR_VER
> Why not exposing all the information in CatalogUpdateVersionInfo?
Good suggestion added a new function CatalogUpdateMetrics in 
CatalogUpdateVersionInfo



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:54:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8949 )

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/service/impala-server.h@978
PS2, Line 978: escriptorMap known_b
> UpdateCatalogVersionMetrics()
Changed the name of function.


http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/service/impala-server.cc@1325
PS2, Line 1325: ImpaladMetrics::CATALOG_VERSION->set_value(catalog_version);
> Maybe my comment was not clear. What I meant is that you should also expose
I have made changes to expose catalog_topic version along with 
catalog_service_id


http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/8949/2/be/src/util/impalad-metrics.h@113
PS2, Line 113: CATALOG_VERSION;
> CATALOG_VERSION, CATALOG_TOPIC_VERSION, CATALOG_SERVICE_ID
Done


http://gerrit.cloudera.org:8080/#/c/8949/2/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/8949/2/common/thrift/metrics.json@233
PS2, Line 233: The version of catalog which is currently with impal
> Maybe "Version of the Impalad catalog."
Changed



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Thu, 18 Jan 2018 19:22:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..

IMPALA-6075: Add Impala daemon metric for catalog version.

This patch adds a new metric that shows the current version of catalog
which is currently used by impala daemon.

Testing:
Verified manually that the new metric for catalog version is displayed and it
corresponds to the latest version in catalogd.INFO log file.

Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
5 files changed, 67 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Hello Dimitris Tsirogiannis, Tim Armstrong,

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

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

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

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..

IMPALA-6075: Add Impala daemon metric for catalog version.

This patch adds a new metric that shows the current version of catalog
which is currently used by impala daemon.

Testing:
Verified manually that the new metric for catalog version is displayed and it
corresponds to the latest version in catalogd.INFO log file.

Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
5 files changed, 28 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  Ran the fuzz test on a private build without failures/crash.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
8 files changed, 160 insertions(+), 35 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8936/2/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8936/2/be/src/exec/hdfs-rcfile-scanner.cc@177
PS2, Line 177: ss << "Codec bad, corrupted ";
> Can you include a bit more detail, i.e. mention that it's RCFile and includ
Done


http://gerrit.cloudera.org:8080/#/c/8936/2/be/src/exec/hdfs-rcfile-scanner.cc@337
PS2, Line 337: ss << "Invalid bytes read col_idx: " << col_idx;
> This could also do with a bit more detail.
Added more detail to describe the error


http://gerrit.cloudera.org:8080/#/c/8936/2/be/src/exec/hdfs-rcfile-scanner.cc@344
PS2, Line 344: void HdfsRCFileScanner::GetCurrentKeyBuffer(int col_idx, bool 
skip_col_data,
> How does this avoid buffer overflows if we don't pass in the length of the
Added length of the buffer and DCHECK to prevent reading beyond the buffer size.


http://gerrit.cloudera.org:8080/#/c/8936/2/be/src/exec/hdfs-rcfile-scanner.cc@348
PS2, Line 348: GetVInt
> These GetVInt() and GetVLong() interfaces seems fundamentally unsafe - they
Changed all callers of GetVInt/GetVLong to pass length of the buffer which can 
be used to check out of bound access.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 16 Jan 2018 21:10:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9147


Change subject: IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the jira.

The problem is fixed by having an ignoreCase match of the column names.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Testing: The fix is verified against the test case in JIRA that fails.
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
1 file changed, 10 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the jira.

The problem is fixed by having an ignoreCase match of the column names.

 Testing
 ---
 Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
2 files changed, 32 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the Jira.

The problem is fixed by having an ignoreCase match of the column names.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
2 files changed, 27 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..


Patch Set 3:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/9147/2//COMMIT_MSG@12
PS2, Line 12: j
> nit: Jira
Done


http://gerrit.cloudera.org:8080/#/c/9147/2//COMMIT_MSG@16
PS2, Line 16:  Testing
> nit: remove the indent here
Done


http://gerrit.cloudera.org:8080/#/c/9147/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@396
PS2, Line 396:   return true;
> nit: if and return could go on a single line now
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:10:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of 
columns
..


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/9147/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-6444: CTAS STORED AS KUDU not supporting reordering of 
columns
> The summary should contain what the change does, not what was broken.
Changed the summary to reflect the support of re-ordering of columns


http://gerrit.cloudera.org:8080/#/c/9147/1//COMMIT_MSG@16
PS1, Line 16: Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
> I think the Change-Id line needs to be the last one of the commit message.
My bad restored the comment id to it's location.


http://gerrit.cloudera.org:8080/#/c/9147/1//COMMIT_MSG@17
PS1, Line 17: Testing: The fix is verified against the test case in JIRA that 
fails.
> Can you add an automated test for this?
Added a test case.


http://gerrit.cloudera.org:8080/#/c/9147/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@393
PS1, Line 393: if (result == false) {
> "if (!result)" ?
Done


http://gerrit.cloudera.org:8080/#/c/9147/1/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@396
PS1, Line 396:   result = true;
> Why not "return true?"
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Mon, 29 Jan 2018 22:58:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the jira.

The problem is fixed by having an ignoreCase match of the column names.

 Testing
 ---
 Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
2 files changed, 29 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the Jira.

The problem is fixed by having an ignoreCase match of the column names.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
2 files changed, 24 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9147/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@393
PS4, Line 393:   if (col.equalsIgnoreCase(name)) return true;
> yes but the first one is a fast path and this one is a slow path so I didn'
Removed the previous check



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:52:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key column names.

[Pranay Singh (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6454: CTAS into Kudu fails with mixed-case partition or 
primary key column names.
..

IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key 
column names.

CTAS into Kudu fails if the primary key and/or the partition column names are 
not
specified in lower case.The problem is that we pass in the primary key column 
names
directly from the parser instead we should be passing the post-analysis 
ColumnDefs
as primary keys. So it is fixed by making changes to createCtasTarget()
in KuduTable class to take a list of ColumnDef class associated with the 
primary keys
from getPrimaryKeyColumnDefs() in CreateTableStmt class.

ColumnDef class has column names stored in lower case that are used by 
createCtasTarget()
to populate primaryKeyColumnNames_ in KuduTable class that resolves the issue.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
4 files changed, 14 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key column names.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6454: CTAS into Kudu fails with mixed-case partition or 
primary key column names.
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9147/9/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/9/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@321
PS9, Line 321:   List PrimaryKeyColumnDefs, 
List partitionParams) {
> style: PrimaryKeyColumnDefs -> primaryKeyColumnDefs
Done


http://gerrit.cloudera.org:8080/#/c/9147/9/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@327
PS9, Line 327: for (ColumnDef pk_coldef: PrimaryKeyColumnDefs) {
> pk_coldef -> pkColDef
Done


http://gerrit.cloudera.org:8080/#/c/9147/9/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

http://gerrit.cloudera.org:8080/#/c/9147/9/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test@290
PS9, Line 290: 
> revert
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Thu, 01 Feb 2018 00:03:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9147/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@393
PS4, Line 393: if (!result) {
> Sorry for the late addition. I think the first check is not needed anymore.
yes but the first one is a fast path and this one is a slow path so I didn't 
remove the former check



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:46:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key column names.

[Pranay Singh (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6454: CTAS into Kudu fails with mixed-case partition or 
primary key column names.
..

IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key 
column names.

CTAS into Kudu fails if the primary key and/or the partition column names are 
not
specified in lower case.The problem is that we pass in the primary key column 
names
directly from the parser instead we should be passing the post-analysis 
ColumnDefs
as primary keys. So it is fixed by making changes to createCtasTarget()
in KuduTable clqss to take a list of ColumnDef class associated with the 
primary keys
from getPrimaryKeyColumnDefs() in CreateTableStmt class.

ColumnDef class has column names stored in lower case that are used by 
createCtasTarget()
to populate primaryKeyColumnNames_ in KuduTable class that resolves the issue.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
5 files changed, 16 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 7
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9147/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@391
PS5, Line 391:   public boolean isPrimaryKeyColumn(String name) {
> Thanks for checking. To address this bug I think we should make the followi
Implemented the suggested changes, thanks for detailed explaination


http://gerrit.cloudera.org:8080/#/c/9147/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/9147/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1678
PS6, Line 1678: // CTAS into Kudu tables with primary key specified in 
upper case.
> IMPALA-6444: CTAS into Kudu table with primary key specified in upper case.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Wed, 31 Jan 2018 05:18:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key column names.

[Pranay Singh (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6454: CTAS into Kudu fails with mixed-case partition or 
primary key column names.
..

IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key 
column names.

CTAS into Kudu fails if the primary key and/or the partition column names are 
not
specified in lower case.The problem is that we pass in the primary key column 
names
directly from the parser instead we should be passing the post-analysis 
ColumnDefs
as primary keys. So it is fixed by making changes to createCtasTarget()
in KuduTable class to take a list of ColumnDef class associated with the 
primary keys
from getPrimaryKeyColumnDefs() in CreateTableStmt class.

ColumnDef class has column names stored in lower case that are used by 
createCtasTarget()
to populate primaryKeyColumnNames_ in KuduTable class that resolves the issue.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
5 files changed, 16 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key column names.

[Pranay Singh (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6454: CTAS into Kudu fails with mixed-case partition or 
primary key column names.
..

IMPALA-6454: CTAS into Kudu fails with mixed-case partition or primary key 
column names.

CTAS into Kudu fails if the primary key and/or the partition column names are 
not
specified in lower case.The problem is that we pass in the primary key column 
names
directly from the parser instead we should be passing the post-analysis 
ColumnDefs
as primary keys. So it is fixed by making changes to createCtasTarget()
in KuduTable class to take a list of ColumnDef class associated with the 
primary keys
from getPrimaryKeyColumnDefs() in CreateTableStmt class.

ColumnDef class has column names stored in lower case that are used by 
createCtasTarget()
to populate primaryKeyColumnNames_ in KuduTable class that resolves the issue.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
5 files changed, 15 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..

IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

In the function KuduTable.isPrimaryKeyColumn() primaryKeyColumnNames_ does not
check for a matching case which causes primaryKeyExprs_ to be empty. This 
causes to
hit an assertion in InsertStmt.prepareExpressions() that generates the exception
reported in the Jira.

The problem is fixed by having an ignoreCase match of the column names.

Testing
---
Verified against the newly added test case that reproduces the issue without 
the fix.

Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
---
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
3 files changed, 12 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6444: CTAS STORED AS KUDU to support reordering of columns

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9147 )

Change subject: IMPALA-6444: CTAS STORED AS KUDU to support reordering of 
columns
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9147/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

http://gerrit.cloudera.org:8080/#/c/9147/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@391
PS5, Line 391:   public boolean isPrimaryKeyColumn(String name) {
> Were you able to reproduce the reported issue?
Yes the issue is reproducible without the fix, when the new CTAS test case is 
run it fails.

I checked the code to see how primaryKeyColumnNames_  is being populated it 
appears that there is no explicit conversion to convert it to lower case, it 
seems to be populated by parser. So it is directly taking the case specified by 
the user and comparing it against lower case (Column.getName())

CreateTableAsSelectStmt.analyze (line# 193)
 +--KuduTable.createCtasTarget (line #327)
  -->createStmt_.getTblPrimaryKeyColumnNames()
   -->TableDef.primaryKeyColNames_
  { This in turn seems to be populated by the parser }


http://gerrit.cloudera.org:8080/#/c/9147/5/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

http://gerrit.cloudera.org:8080/#/c/9147/5/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test@292
PS5, Line 292: create table partitioned_kudu_tbl
> Bug was hit in the FE so an Analyzer test in AnalyzeDDL#TestCreateTableAsSe
Moved over the test case to AnalyzeDDLTest.java



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica1c8ec1544339e9e80733a7a0c78594e0a727d2
Gerrit-Change-Number: 9147
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Tue, 30 Jan 2018 07:05:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8949 )

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..


Patch Set 8:

> Looks like the API for Metric changed
 >
 > -fsanitize=thread -DTHREAD_SANITIZER -fverbose-asm -D_GNU_SOURCE
 > -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
 > -fPIC   -fPIC -MD -MT 
 > be/src/service/CMakeFiles/Service.dir/impala-server.cc.o
 > -MF be/src/service/CMakeFiles/Service.dir/impala-server.cc.o.d -o
 > be/src/service/CMakeFiles/Service.dir/impala-server.cc.o -c
 > be/src/service/impala-server.cc
 > 01:58:23 ] be/src/service/impala-server.cc:1325:36: error: no
 > member named 'set_value' in 
 > 'impala::AtomicMetric';
 > did you mean 'SetValue'?
 > 01:58:23 ]   ImpaladMetrics::CATALOG_VERSION->set_value(catalog_version);
 > 01:58:23 ]^
 > 01:58:23 ]SetValue
 > 01:58:23 ] be/src/util/metrics.h:233:8: note: 'SetValue' declared
 > here
 > 01:58:23 ]   void SetValue(const int64_t& value) {
 > value_.Store(value); }
 > 01:58:23 ]^
 > 01:58:23 ] be/src/service/impala-server.cc:1326:42: error: no
 > member named 'set_value' in 
 > 'impala::AtomicMetric';
 > did you mean 'SetValue'?
 > 01:58:23 ]   
 > ImpaladMetrics::CATALOG_TOPIC_VERSION->set_value(catalog_topic_version);
 > 01:58:23 ]  ^
 > 01:58:23 ]  SetValue
 > 01:58:23 ] be/src/util/metrics.h:233:8: note: 'SetValue' declared
 > here
 > 01:58:23 ]   void SetValue(const int64_t& value) {
 > value_.Store(value); }
 > 01:58:23 ]^
 > 01:58:23 ] be/src/service/impala-server.cc:1327:39: error: no
 > member named 'set_value' in 'impala::LockedMetric impala::TMetricKind::type::PROPERTY>'; did you mean 'SetValue'?
 > 01:58:23 ]   
 > ImpaladMetrics::CATALOG_SERVICE_ID->set_value(PrintId(catalog_service_id));
 > 01:58:23 ]   ^
 > 01:58:23 ]   SetValue
 > 01:58:23 ] be/src/util/metrics.h:197:8: note: 'SetValue' declared
 > here
 > 01:58:23 ]   void SetValue(const T& value) {
 > 01:58:23 ]^
 > 01:58:23 ] 3 errors generated.
 > 01:58:23 ] ninja: build stopped: subcommand failed.
 > 01:58:23 ] Error in /home/ubuntu/Impala/bin/make_impala.sh at line
 > 178: ${MAKE_CMD} ${MAKE_ARGS}

Yes the API for METRIC changed that caused it to fail I have made the correction


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 06 Feb 2018 01:42:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Hello Dimitris Tsirogiannis, Tim Armstrong, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..

IMPALA-6075: Add Impala daemon metric for catalog version.

This patch adds new metrics for current version of catalog, current catalog 
topic version
and catalog service id which are currently used by impala daemon.

Testing:
Verified manually that the new metrics for catalog version, catalog topic 
version,
catalog service id are displayed and they corresponds to the latest version in
catalogd.INFO log file.

Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
5 files changed, 62 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 242 insertions(+), 61 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/hdfs-rcfile-scanner.cc@119
PS5, Line 119: ss << stream_->filename() << " Invalid 
RCFILE_VERSION_HEADER: '"
> I meant the filename of the file being decoded, i.e. stream_->filename().
Done


http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/hdfs-rcfile-scanner.cc@119
PS5, Line 119: ss << stream_->filename() << " Invalid 
RCFILE_VERSION_HEADER: '"
> It seems like the message text should be enough to distinguish, right?
Done


http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h
File be/src/exec/scanner-context.inline.h:

http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h@96
PS5, Line 96:   }
> Is this possible? The intent of the above code is that it shouldn't go nega
Done


http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h@97
PS5, Line 97:
> This will crash since the template for the error message in common/thrift/g
This is not needed as you've suggested above I have introduced the DCHECK in 
the beginning to check that length is never negative



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Sat, 10 Feb 2018 01:01:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8936


Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  Ran the fuzz test on a private build without failures/crash.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
4 files changed, 25 insertions(+), 7 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker, Zoram Thanga, Todd Lipcon,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/common/init.cc
M be/src/statestore/statestored-main.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 50 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 7
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc
File be/src/catalog/catalogd-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@62
PS6, Line 62: HandlerTerm
> "HandleSigTerm"?
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@63
PS6, Line 63:   LOG(INFO) << "Caught SIGTERM daemon going down" << endl;
> nit: please format this more grammatically. For example "Caught SIGTERM. Da
Changed the log message and removed the endl , looks like the messages are 
being flushed perhaps by atexit hook as you say. Since the testcase check for 
the message "Caught SIGTERM" which seems to be working fine.


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@64
PS6, Line 64:   exit(0);
> Does this change the behavior? I.e., would we have exited with 0 before, to
This exit will ensure a normal process termination, if I don't use it the 
process will not be terminated.


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@120
PS6, Line 120:   // We want to log a message when catalogd is
> nit: wrap comments at 90 chars, here and elsewhere.
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@125
PS6, Line 125:   sigaction(SIGTERM, , nullptr);
> why not set this earlier? seems like there could be a relatively long start
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc@60
PS6, Line 60: // doing an exit.
> This code is now copy-pasted to several files. Can you dedup it?
Removed the code block and moved it to the common handler


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc@104
PS6, Line 104:   // We want to log a message when impalad is
> same here
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/util/minidump.cc
File be/src/util/minidump.cc:

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/util/minidump.cc@101
PS6, Line 101:   LOG(INFO) << "Signal received SIGUSR1" << endl;
> The other messages are "Caught SIGTERM daemon going down". Can you make the
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/tests/custom_cluster/test_breakpad.py
File tests/custom_cluster/test_breakpad.py:

http://gerrit.cloudera.org:8080/#/c/10847/6/tests/custom_cluster/test_breakpad.py@239
PS6, Line 239: self.assert_impalad_log_contains('INFO', 'Caught SIGTERM 
daemon going down',
> Is there a way to also check the exit code of the processes?
Checked the code for exit of the process, could not find the function where the 
exit of the process is checked.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 6
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 03 Aug 2018 23:29:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
3 files changed, 50 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Andrew Sherman, Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public 
Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 56 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 8:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc
File be/src/catalog/catalogd-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@58
PS6, Line 58: using namespace apache::thrift;
> If you make more changes to this file then you could remove this duplicated
Done


http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@64
PS6, Line 64:   InitFeSupport();
> Without your change, what was the exit code when Impala caught a SIGTERM?
I'm not able to find what was the exit code when Impala caught a SIGTERM out 
but as per convention exit code 0 is success/desirable and other exit codes are 
not desirable/failure.


http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@177
PS8, Line 177: // doing an exit.
> nit: wrap at 90c, please also check the rest of the change
Done


http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@291
PS8, Line 291: catalogd
> This does not seem specific to the catalog.
Updated the comment to include impalad/statestored/catalogd


http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@291
PS8, Line 291:   // Signal handler for handling the SIGTERM. We want to log a 
message when catalogd is
 :   // being shutdown using a SIGTERM.
 :   struct sigaction action;
 :   memset(, 0, sizeof(struct sigaction));
 :   action.sa_sigaction = 
 :   sigaction(SIGTERM, , nullptr);
> Can this whole block be moved to earlier in the initialization? It takes a
I have moved it further up as per Todd's comment,  log messages won't be 
printed if I move the code block further up.


http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/util/minidump.cc
File be/src/util/minidump.cc:

http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/util/minidump.cc@101
PS8, Line 101:   LOG(INFO) << "Caught signal: SIGUSR1" << endl;
> This should get the signal from the parameter. You could use the same log m
Done


http://gerrit.cloudera.org:8080/#/c/10847/8/tests/custom_cluster/test_breakpad.py
File tests/custom_cluster/test_breakpad.py:

http://gerrit.cloudera.org:8080/#/c/10847/8/tests/custom_cluster/test_breakpad.py@239
PS8, Line 239: self.assert_impalad_log_contains('INFO', 'Caught signal: 
SIGTERM. Daemon will exit',
> Can you test that the log also contains the sending PID?
I tried testing for PID, the problem is that signal is sent from a subprocess 
so can't get it's PID



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 07 Aug 2018 21:29:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/util/minidump.cc
File be/src/util/minidump.cc:

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/util/minidump.cc@119
PS10, Line 119:   sigaction(SIGUSR1, _action, NULL);
> While you're here, maybe check the return value to make sure this was succe
oldact->sa_handler has a value 1 so can't have a check for nullptr



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Aug 2018 23:31:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 10:

(2 comments)

> (4 comments)

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc@179
PS10, Line 179:  :
> nit: space on wrong side of :
Done


http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc@297
PS10, Line 297:   sigaction(SIGTERM, , nullptr);
> We should at least check the return value here. We also should pass an empt
There is a value 1 displayed for old_action.sa_handler so can't have a nullptr 
check that will hit the assert



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Aug 2018 23:30:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc@297
PS10, Line 297:   if (sigaction(SIGTERM, , nullptr) == -1) {
> Where does it display? Does that mean that we are overwriting a previous si
I think that value corresponds to default signal handler constant value of 1, 
2, 3 are default signal values. e.g

https://www.ibm.com/support/knowledgecenter/en/ssw_i5_54/apis/sigactn.htm#TBLSIGTBL1



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 12
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 23 Aug 2018 17:43:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Andrew Sherman, Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public 
Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 60 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 12
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 12:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/10/be/src/common/init.cc@297
PS10, Line 297:   if (sigaction(SIGTERM, , nullptr) == -1) {
> > There is a value 1 displayed for old_action.sa_handler ...
I tried printing the old action sa_handler in impalad.INFO and a value of 1 was 
displayed for sa_handler (old action). The value 1 corresponds to default 
action for SIGTERM, I don't think there is a need for validating it because 
default behavior of SIGTERM would result in killing of process without core 
dump and that behavior is not altered.


http://gerrit.cloudera.org:8080/#/c/10847/12/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/12/be/src/common/init.cc@298
PS12, Line 298: "
> Ping?
Added GetStrErrMsg()



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 12
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 24 Aug 2018 19:20:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Andrew Sherman, Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public 
Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 60 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/10847/13
--
To view, visit http://gerrit.cloudera.org:8080/10847
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 13
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Andrew Sherman, Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public 
Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 64 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/10847/15
--
To view, visit http://gerrit.cloudera.org:8080/10847
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 15
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10847/14/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/10847/14/be/src/common/init.cc@298
PS14, Line 298:
> Should we consider aborting the process if we fail to install the signal ha
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 15
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 27 Aug 2018 19:08:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Andrew Sherman, Lars Volker, Zoram Thanga, Todd Lipcon, Impala Public 
Jenkins,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/common/init.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
4 files changed, 60 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 11
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10587 )

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10587/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/10587/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1251
PS3, Line 1251:  RELOAD_PARTITION_METADATA if not set in flags results in not 
getting the
  :* partition details from HiveMetaStore in 
updatePartitionsFromHms().
> nit: this double negative is a bit confusing. Perhaps it's clearer to rephr
Changed the comment, thanks for rewording it.


http://gerrit.cloudera.org:8080/#/c/10587/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1254
PS3, Line 1254: or if there are no partitions in the table
> I'm not sure I follow this part. How does the caller know if there are no p
That's right the unpartitioned case doesn't have to consider this, I have 
removed the sentence.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
Gerrit-Change-Number: 10587
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 20 Jul 2018 18:57:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Balazs Jeszenszky, Todd Lipcon, Vuk Ercegovac, 

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

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

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

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations
..

IMPALA-6994: Avoid reloading a table's HMS data for file-only operations

The problem is that while inserting a new row to an existing partition
of a HDFS table or to an unpartitioned table, catalogd makes an unnecessary
call to Hive MetaStore to do a getTable() and list the partitions of the
table. In such a case where a row is being inserted to an existing
partition or to an unpartitioned table only the file metadata for
HDFS tables needs to be updated.

This change avoids calling the Hive Meta Store in updatePartitionsFromHms()
by providing a hint from the caller, to skip loading the partitions.

Testing:
  Ran core test without failure.

Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
2 files changed, 126 insertions(+), 52 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
Gerrit-Change-Number: 10587
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6214: Determine and warn about stuck fragment instances.

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11021


Change subject: IMPALA-6214: Determine and warn about stuck fragment instances.
..

IMPALA-6214: Determine and warn about stuck fragment instances.

In order to diagnose query hangs, we need to know the fragment execution
time on a particular exec node. Inspecting the query run time profile
to find the cause of hang does not give much details.

This change helps in finding the problematic 'exec node' where the fragment
execution is not making progress. The change makes use of kudu watchdog that
periodically polls and prints the delay in response from an exec node.

Testing:

a) Added a delay on the sender side as a part of manual test case to notice
   the affect of change. The watchdog prints the detail of fragmentID and
   nodeID when the watchdog timer expires.

b) Ran the core test without failure.

Change-Id: I260a1d0a3477e5c6a46094e664500c3e2ed7de62
---
M be/src/common/global-flags.cc
M be/src/runtime/krpc-data-stream-recvr.cc
2 files changed, 27 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I260a1d0a3477e5c6a46094e664500c3e2ed7de62
Gerrit-Change-Number: 11021
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6214: Determine and warn about stuck fragment instances.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11021 )

Change subject: IMPALA-6214: Determine and warn about stuck fragment instances.
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11021/1/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/11021/1/be/src/runtime/krpc-data-stream-recvr.cc@230
PS1, Line 230:   stringstream dstid;
 :
 :   dstid << recvr_->dest_node_id();
 :   string frgId = PrintId(recvr_->fragment_instance_id());
 :   string watchdog_str = "fragment_instance_id= " + frgId + " 
node= " + dstid.str();
 :   memset(buf, BUFSZ, 0);
 :   sprintf(buf, "%s : %d  %s", __FILE__, __LINE__, 
watchdog_str.c_str());
> constructing this once per batch seems like a big performance red flag.
I'll remove it.


http://gerrit.cloudera.org:8080/#/c/11021/1/be/src/runtime/krpc-data-stream-recvr.cc@239
PS1, Line 239:   kudu::SetStackTraceSignal(SIGRTMIN + 1);
> you should call this at init time, not once per batch. I'm not even sure it
I'll remove it.


http://gerrit.cloudera.org:8080/#/c/11021/1/be/src/runtime/krpc-data-stream-recvr.cc@266
PS1, Line 266:   data_arrival_cv_.wait(l);
> maybe it would be simpler to just use a timed-wait variant here to periodic
Thanks Todd I was thinking along the same lines, it's a good and easy fix for 
this case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I260a1d0a3477e5c6a46094e664500c3e2ed7de62
Gerrit-Change-Number: 11021
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:14:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6214: Determine and warn about stuck fragment instances.

[Pranay Singh (Code Review)]

Hello Todd Lipcon, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6214: Determine and warn about stuck fragment instances.
..

IMPALA-6214: Determine and warn about stuck fragment instances.

In order to diagnose query hangs, we need to know the fragment execution
time on a particular exec node. Inspecting the query run time profile
to find the cause of hang does not give much details.

This change helps in finding the problematic 'exec node' where the fragment
execution is not making progress. The change makes use of timed-wait wait
that waits for the 1 min delay if the batch request is not recieved from
the sender node.

Testing:

a) Added a delay on the sender side as a part of manual test case to notice
   the affect of change. The watchdog prints the detail of fragmentID and
   nodeID when the watchdog timer expires.

b) Ran the core test without failure.

Change-Id: I260a1d0a3477e5c6a46094e664500c3e2ed7de62
---
M be/src/runtime/krpc-data-stream-recvr.cc
1 file changed, 7 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I260a1d0a3477e5c6a46094e664500c3e2ed7de62
Gerrit-Change-Number: 11021
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] IMPALA-6994:Avoid reloading a table's HMS data for file-only operations

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10587 )

Change subject: IMPALA-6994:Avoid reloading a table's HMS data for file-only 
operations
..


Patch Set 2:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/10587/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-6994:Avoid reloading a table's HMS data for file-only 
operations
> nit: add a space
Done


http://gerrit.cloudera.org:8080/#/c/10587/2//COMMIT_MSG@10
PS2, Line 10: unecessary
> nit: check spelling and remove extra spaces.
Done


http://gerrit.cloudera.org:8080/#/c/10587/2//COMMIT_MSG@9
PS2, Line 9: unpartitioned HDFS table,
   :   or to an existing partition
> simplify to just HDFS table, or is there some case that's being excluded?
Done


http://gerrit.cloudera.org:8080/#/c/10587/2//COMMIT_MSG@21
PS2, Line 21:   Testing: Ran core test without failure.
> Is there any new scenario that you want to test for this change? If existin
Yes I want to test the scenario where concurrent removal
of partition and insertion of row to the same partition happens.

a) Impala is inserting the row to an existing partition
b) Hive is removing the partition at the same time.


http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1252
PS2, Line 1252: nonewPartitionHint
> needs a comment.
Done


http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1252
PS2, Line 1252: nonewPartitionHint
> needs a comment.
Added the comment


http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1365
PS2, Line 1365:   loadMetadataAndDiskIds(partitionsToUpdateFileMdByPath, 
true);
> any way to refactor this function so you don't need to duplicate this code
Refactored the code and added a new function to reload the partition.


http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/10587/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@640
PS2, Line 640:   private void loadTableMetadata(Table tbl, long 
newCatalogVersion,
> the arguments here are getting quite unwieldly, and it seems that the two e
Made changes to the code to use enum MetadataLoadFlag



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
Gerrit-Change-Number: 10587
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 19 Jul 2018 18:27:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6994: Avoid reloading a table's HMS data for file-only operations

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Todd Lipcon, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6994: Avoid reloading a table's HMS data for file-only 
operations
..

IMPALA-6994: Avoid reloading a table's HMS data for file-only operations

The problem is that while inserting a new row to an existing partition
of a HDFS table or to an unpartitioned table, catalogd makes an unnecessary
call to Hive MetaStore to do a getTable() and list the partitions of the
table. In such a case where a row is being inserted to an existing
partition or to an unpartitioned table only the file metadata for
HDFS tables needs to be updated.

This change avoids calling the Hive Meta Store in updatePartitionsFromHms()
by providing a hint from the caller, to skip loading the partitions.

Testing:
  Ran core test without failure.

Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
---
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
2 files changed, 126 insertions(+), 52 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I331bb0371fde287f43a85b025b4f98cb45f3eb3c
Gerrit-Change-Number: 10587
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

  Currently Impalad does not log any message when SIGTERM is sent to impalad
  to terminate or to do a graceful shutdown. This change logs a message when 
SIGTERM is
  recieved by impalad. This logging will assist in debugging the issues seen in 
the
  field where impalad was not gracefully shutdown.

  Testing:
  ---
a) Used kill to send signals to impalad `kill -s SIGTERM ` 
and see
the message is being logged in impalad.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/service/impalad-main.cc
M be/src/util/minidump.cc
2 files changed, 14 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker, Zoram Thanga,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
5 files changed, 67 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 3
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker, Zoram Thanga,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
5 files changed, 67 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 2:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@9
PS2, Line 9:   Currently Impalad does not log any message when SIGTERM is sent 
to impalad
> nit: flush left
Done


http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@11
PS2, Line 11: recieved
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@12
PS2, Line 12: shutdown
> nit: two words
Done


http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@14
PS2, Line 14:   Testing:
> This change should have an automatic test, possibly in custom-cluster-tests
Done


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@62
PS2, Line 62: static void HandlerTerm(int signum)
> nit: formatting ({ on same line). You can run clang-format with the config
Done


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@64
PS2, Line 64:LOG(INFO) << "SIGTERM encountered invoking default handler 
system going down" << endl;
> I would change the wording to "Caught SIGTERM. Daemon going down."
Done


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@108
PS2, Line 108: old_action
> Do we currently set a handler for SIGTERM elsewhere? If not, we should remo
No we don't handle the SIGTERM elsewhere I have removed the old_action now I 
exit the process after handling SIGTERM


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@108
PS2, Line 108: old_action
> Please add a comment explaining why SIGTERM is specifically handled (i.e.,
Done


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

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/util/minidump.cc@101
PS2, Line 101: recieved
> nit: typo
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Jul 2018 21:33:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Hello Lars Volker, Zoram Thanga,

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

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

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..

IMPALA-6271: Impala daemon should log a message when it's being shut down

Currently Impalad does not log any message when SIGTERM is sent to
impalad to terminate or to do a graceful shut down. This change logs
a message when SIGTERM is received by impalad/catalogd/statestored.
This logging will assist in debugging the issues seen in the field
where impalad was not gracefully shut down (some other signal
was generated that led to impalad/catalogd/statestored crash).

Testing:
---
a) Used kill to send signals to impalad/catalogd/statestored
   `kill -s SIGTERM ` and see the
   log message is being logged in impalad/catalogd/statestored.INFO.
b) Ran test_breakpad.py to check that existing breakpad functionalities
   are not affected.
c) Ran exhaustive tests without failure.
d) Added new test in test_breakpad.py to handle SIGTERM for
   impalad/statestored/catalogd.

Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/minidump.cc
M tests/custom_cluster/test_breakpad.py
5 files changed, 77 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10847 )

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 5:

> Can you also add test case to look for the SIGTERM induced
 > 'shutdown log' in the log file? Since that's the whole point of
 > adding the handler, I think it would be good to ensure we don't
 > lose the log message.

Added the code to check for log message before and after.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Jul 2018 01:08:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] Addressing issues.

[Pranay Singh (Code Review)]

Pranay Singh has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9073


Change subject: Addressing issues.
..

Addressing issues.

Change-Id: I5b1963e3e96ff09f44bf55f6a69eced228aaf3b2
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
5 files changed, 67 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5b1963e3e96ff09f44bf55f6a69eced228aaf3b2
Gerrit-Change-Number: 9073
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] Addressing issues.

[Pranay Singh (Code Review)]

Pranay Singh has abandoned this change. ( http://gerrit.cloudera.org:8080/9073 )

Change subject: Addressing issues.
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5b1963e3e96ff09f44bf55f6a69eced228aaf3b2
Gerrit-Change-Number: 9073
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-6075: Add Impala daemon metric for catalog version.

[Pranay Singh (Code Review)]

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-6075: Add Impala daemon metric for catalog version.
..

IMPALA-6075: Add Impala daemon metric for catalog version.

This patch adds new metrics for current version of catalog, current catalog 
topic version
and catalog service id which are currently used by impala daemon.

Testing:
Verified manually that the new metrics for catalog version, catalog topic 
version,
catalog service id are displayed and they corresponds to the latest version in
catalogd.INFO log file.

Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
5 files changed, 66 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2307eb434561ed74ff058106541c0ebda017d97
Gerrit-Change-Number: 8949
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  Ran the fuzz test on a private build without failures/crash.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 218 insertions(+), 61 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 221 insertions(+), 61 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 7:

> I'm still seeing crashes when I loop this locally:
 > F0306 16:34:04.306205 29911 mem-pool.h:245] Check failed: size >= 0
 > (-9 vs. 0)
 > *** Check failure stack trace: ***
 > @  0x3c32cbd  google::LogMessage::Fail()
 > @  0x3c34562  google::LogMessage::SendToLog()
 > @  0x3c32697  google::LogMessage::Flush()
 > @  0x3c35c5e  google::LogMessageFatal::~LogMessageFatal()
 > @  0x184eeb4  impala::MemPool::TryAllocate()
 > @  0x1be5d08  impala::HdfsRCFileScanner::StartRowGroup()
 > @  0x1be8032  impala::HdfsRCFileScanner::ProcessRange()
 > @  0x296931a  impala::BaseSequenceScanner::GetNextInternal()
 > @  0x1bc71b2  impala::HdfsScanner::ProcessSplit()
 > @  0x1b9f48c  impala::HdfsScanNode::ProcessSplit()
 > @  0x1b9e8b7  impala::HdfsScanNode::ScannerThread()
 > @  0x1b9dd3f  
 > _ZZN6impala12HdfsScanNode22ThreadTokenAvailableCbEPNS_17ThreadResourceMgr12ResourcePoolEENKUlvE_clEv
 > @  0x1b9fcdd  
 > _ZN5boost6detail8function26void_function_obj_invoker0IZN6impala12HdfsScanNode22ThreadTokenAvailableCbEPNS3_17ThreadResourceMgr12ResourcePoolEEUlvE_vE6invokeERNS1_15function_bufferE
 > @  0x17f4ce0  boost::function0<>::operator()()
 > @  0x1af63ff  impala::Thread::SuperviseThread()
 > @  0x1aff3cf  boost::_bi::list5<>::operator()<>()
 > @  0x1aff2f3  boost::_bi::bind_t<>::operator()()
 > @  0x1aff2b6  boost::detail::thread_data<>::run()
 > @  0x2dbc1aa  thread_proxy
 > @ 0x7f5af8f926ba  start_thread
 > @ 0x7f5af8cc841d  clone
 > Picked up JAVA_TOOL_OPTIONS: 
 > -agentlib:jdwp=transport=dt_socket,address=3,server=y,suspend=n
 > Wrote minidump to 
 > /home/tarmstrong/Impala/incubator-impala/logs/cluster/minidumps/impalad/31a8ae22-6a2a-4118-4ff4e1b6-be18ef7c.dmp
 >
 > How about we leave the test disabled and move forward with the
 > improvements, then we can finish up the remaining fixes needed
 > later?

Shall I revert my changes to tests/query_test/test_scanners_fuzz.py ?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 7
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 07 Mar 2018 18:56:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8936/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8936/10//COMMIT_MSG@9
PS10, Line 9: Enabled the fuzz test for Sequence and RCFiles and added new 
checks to return
> Commit message is stale.
Fixed it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 13 Mar 2018 15:38:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Introduced new error message when scanning a corrupt Sequence or RCFile.
Added new checks to detect buffer overrun while handling Sequence or RCFile.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
8 files changed, 233 insertions(+), 58 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 11
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 242 insertions(+), 61 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 12
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8936 )

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 12:

> Uploaded patch set 12.

Tim please ignore these changes.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 12
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 13 Mar 2018 23:35:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
8 files changed, 233 insertions(+), 58 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 10
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
M tests/query_test/test_scanners_fuzz.py
9 files changed, 234 insertions(+), 59 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Pranay Singh (Code Review)]

Hello anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..

IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

Enabled the fuzz test for Sequence and RCFiles and added new checks to return
when failure is encountered while reading an RC file or sequence file.

Testing:
  a) Ran the fuzz test on a private build without failures/crash.
  b) Ran end to end test, back end tests with new changes.

Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
---
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/read-write-util-test.cc
M be/src/exec/read-write-util.h
M be/src/exec/scanner-context.inline.h
M be/src/util/decompress.cc
8 files changed, 234 insertions(+), 58 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] DRAFT IMPALA-5973: Provide query plan in JSON format.

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9614 )

Change subject: DRAFT IMPALA-5973: Provide  query plan in JSON format.
..


Patch Set 1:

> - High-level comment: It seems kind of redundant that we build the
 > explain string and Map at the same time for every getExplainString*().
 > Maybe we should have an intermediate state that we can convert to
 > both text-plan and JSON depending on the caller.  Seems like Map
 > would be the obvious choice (guava has helper methods to build the
 > explain string from Map) and obviously we can build a JSON string.
 >
 > - Please add a web endpoint for the json profile (something like
 > query_profile?json) to make it easy to consume and write e-e tests.
 >
 > Alex, does the approach sound ok, or do you think it can be done in
 > any better way?

Forgot to mention that String will be removed later, I have only added the new 
code and not removed the old code for verification purpose. Since this is the 
draft change I'll do the cleanup if the approach is fine.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7429ac1e93a9c524233383cafd387056fcf2542d
Gerrit-Change-Number: 9614
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Wed, 14 Mar 2018 16:35:51 +
Gerrit-HasComments: No