[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

2017-08-14 Thread Kim Jin Chul (Code Review)
Kim Jin Chul has posted comments on this change.

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..


Patch Set 5: Code-Review+1

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7414/5/be/src/gutil/hash/hash.h
File be/src/gutil/hash/hash.h:

Line 101
Unnecessary include because of std::hash


Line 117
MSVC stuff is removed


Line 242
MSVC stuff is removed


Line 305
MSVC stuff is removed


Line 318
MSVC stuff is removed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5116: Remove deprecated hash * types in gutil

2017-08-14 Thread Kim Jin Chul (Code Review)
Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5116: Remove deprecated hash_* types in gutil
..

IMPALA-5116: Remove deprecated hash_* types in gutil

The following class templates are substituted from C++11 standard.
__gnu_cxx::hash_map => std::unordered_map
__gnu_cxx::hash_set => std::unordered_set
__gnu_cxx::hash => std::hash

Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
---
M be/src/gutil/hash/hash.cc
M be/src/gutil/hash/hash.h
M be/src/gutil/strings/join.h
M be/src/gutil/strings/serialize.cc
M be/src/gutil/strings/serialize.h
M be/src/gutil/strings/split.cc
M be/src/gutil/strings/split.h
7 files changed, 37 insertions(+), 185 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06af30fd15acd43a60e3c30af54056c96520b6e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5109: Increase range of backend latency histogram

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-5109: Increase range of backend latency histogram
..

IMPALA-5109: Increase range of backend latency histogram

Tiny change to increase range of backend start-up latency histogram to
30 minutes, with 4 s.f. of precision. This allows us to capture
situations where some backend is very slow to start with more
discrimination than the previous 20s upper bound.

Change-Id: I7b6f39c0f769b5b590009d47f5b2e904f48fae8d
---
M be/src/runtime/coordinator.cc
1 file changed, 2 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7b6f39c0f769b5b590009d47f5b2e904f48fae8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 6: Code-Review+2

Fix a couple of clang-tidy warnings.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Henry Robinson (Code Review)
Hello Impala Public Jenkins, Sailesh Mukil, Dan Hecht,

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

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

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

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..

IMPALA-5743: Support TLS version configuration for Thrift servers

* Add --ssl_minimum_version which controls the minimum SSL/TLS version
  that clients and servers will use when negotiating a secure
  connection.
* Two kinds of version specification are allowed: 'TLSv1.1' enables
  TLSv1.1 and all subsequent verisons. 'TLSv1.1_only' enables only
  TLSv1.1. The latter is not exposed in user-facing text as it is
  typically only used for testing.
* Handle case where platform may not support TLSv1.1 or v1.2 by checking
  OpenSSL version number.
* Bump Thrift toolchain version to -p10.

Testing:
* New tests in thrift-server-test.cc. In particular, test all 36
  configurations of client and server protocol versions, and ensure that
  the expected successes or failures are seen.

Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
---
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M bin/impala-config.sh
10 files changed, 215 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 5: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1056/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

2017-08-14 Thread Henry Robinson (Code Review)
Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2
..

IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

The shell uses Thrift's TSSLSocket to negotiate secure connections to
Impala. This socket uses a variable SSL_VERSION to determine which SSL
and TLS protocol versions it will connect to.

SSL_VERSION was hardcoded to be PROTOCOL_TLSv1, which only supports
TLSv1 servers and no other protocol version. Change the allowed version
to be PROTOCOL_SSLv23, which supports any TLS or SSL protocol. We rely
on the server not to allow SSLv2 or v3 connections.

Testing: Added a new custom cluster test to confirm that the shell can
connect to a TLSv1.2 cluster.

Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
---
M shell/TSSLSocketWithWildcardSAN.py
M tests/custom_cluster/test_client_ssl.py
M tests/util/thrift_util.py
3 files changed, 26 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7675/1/tests/custom_cluster/test_client_ssl.py
File tests/custom_cluster/test_client_ssl.py:

Line 103: 
> Add:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..


IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.

Fix: If the source URI does not have write permissions we get
 a "READ WRITE warning" on CREATE TABLE LIKE PARQUET.
 During analyze() in the class CreateTableLikeFileStmt we should
 be checking only for "READ" permission for the source URI
 not for READ and WRITE as the source URI should be readable.

Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Reviewed-on: http://gerrit.cloudera.org:8080/7671
Reviewed-by: Sailesh Mukil 
Reviewed-by: Bharath Vissapragada 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Impala Public Jenkins: Verified
  Bharath Vissapragada: Looks good to me, approved
  Sailesh Mukil: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-1478: Improve error message when subquery is used in the ON clause

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-1478: Improve error message when subquery is used in the 
 ON clause
..


IMPALA-1478: Improve error message when subquery is used in the
 ON clause

Fix: Print the error stating that "Suquery not allowed in the ON clause"
 Add test case for testing the failure when a subquery is used in
 the ON clause.

Change-Id: I0d1dc47987de7ea04402e1ead31d81cddf2f96f2
Reviewed-on: http://gerrit.cloudera.org:8080/7588
Reviewed-by: Bharath Vissapragada 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/TableRef.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 9 insertions(+), 0 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Bharath Vissapragada: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I0d1dc47987de7ea04402e1ead31d81cddf2f96f2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pranay Singh


[Impala-ASF-CR] IMPALA-1478: Improve error message when subquery is used in the ON clause

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-1478: Improve error message when subquery is used in the 
 ON clause
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0d1dc47987de7ea04402e1ead31d81cddf2f96f2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

2017-08-14 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7675/1/tests/custom_cluster/test_client_ssl.py
File tests/custom_cluster/test_client_ssl.py:

Line 103: 
Add:
@pytest.mark.xfail(run=True, reason="IMPALA-4295 on Centos6")

Certain wildcard certs don't work on CentOS 6.

It still runs and passes on other platforms.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2
..

IMPALA-5775: Allow shell to support TLSv1, v1.1 and v1.2

The shell uses Thrift's TSSLSocket to negotiate secure connections to
Impala. This socket uses a variable SSL_VERSION to determine which SSL
and TLS protocol versions it will connect to.

SSL_VERSION was hardcoded to be PROTOCOL_TLSv1, which only supports
TLSv1 servers and no other protocol version. Change the allowed version
to be PROTOCOL_SSLv23, which supports any TLS or SSL protocol. We rely
on the server not to allow SSLv2 or v3 connections.

Testing: Added a new custom cluster test to confirm that the shell can
connect to a TLSv1.2 cluster.

Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
---
M shell/TSSLSocketWithWildcardSAN.py
M tests/custom_cluster/test_client_ssl.py
M tests/util/thrift_util.py
3 files changed, 25 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5487f82d110676b9c3c7a5305931da00c7f68ca0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propagate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Reviewed-on: http://gerrit.cloudera.org:8080/7574
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5776: Write partial tuple to the correct mempool

2017-08-14 Thread Taras Bobrovytsky (Code Review)
Taras Bobrovytsky has uploaded a new patch set (#4).

Change subject: IMPALA-5776: Write partial tuple to the correct mempool
..

IMPALA-5776: Write partial tuple to the correct mempool

In the text scanner, we were writing the partial tuple variable length
data to data_buffer_pool_ mempool which caused strange behavior, such
as incorrect results.

If we are scanning compressed data, the pool gets attached to the row
batch at the end of a GetNext() call and gets freed before the next
GetNext() call. This is wrong because we expect the data in the partial
tuple to survive between the GetNext() calls. If we are scanning non
compressed data, data_buffer_pool_ never gets cleared and grows over
time until the scanner finishes reading the scan range.

We fix the problem by creating a new memory pool for the partial tuple
constant and variable length data. The new memory pool does not hold
tuple data of returned batches because the data is always deep-copied
into the output batch.

Testing:
- Updated test_scanners_fuzz.py to make slightly more significant
  changes to the data files.
- Ran scanner tests locally on ASAN build.
- No new tests were added, because it is difficult to construct test
  cases due to the issue being non-deterministic.

Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
---
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M tests/query_test/test_scanners_fuzz.py
3 files changed, 46 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5776: Write partial tuple to the correct mempool

2017-08-14 Thread Taras Bobrovytsky (Code Review)
Taras Bobrovytsky has uploaded a new patch set (#4).

Change subject: IMPALA-5776: Write partial tuple to the correct mempool
..

IMPALA-5776: Write partial tuple to the correct mempool

In the text scanner, we were writing the partial tuple variable length
data to data_buffer_pool_ mempool which caused strange behavior, such
as incorrect results.

If we are scanning compressed data, the pool gets attached to the row
batch at the end of a GetNext() call and gets freed before the next
GetNext() call. This is wrong because we expect the data in the partial
tuple to survive between the GetNext() calls. If we are scanning non
compressed data, data_buffer_pool_ never gets cleared and grows over
time until the scanner finishes reading the scan range.

We fix the problem by creating a new memory pool for the partial tuple
constant and variable length data. The new memory pool does not hold
tuple data of returned batches because the data is always deep-copied
into the output batch.

Testing:
- Updated test_scanners_fuzz.py to make slightly more significant
  changes to the data files.
- Ran scanner tests locally on ASAN build.
- No new tests were added, because it is difficult to construct test
  cases due to the issue being non-deterministic.

Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
---
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M tests/query_test/test_scanners_fuzz.py
3 files changed, 46 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5776: Write partial tuple to the correct mempool

2017-08-14 Thread Taras Bobrovytsky (Code Review)
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5776: Write partial tuple to the correct mempool
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7639/3/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

Line 804:   // Copy over all materialized slots in the partial tuple.
> Only describes what the code does. How about something like:
Done


Line 808:   // prevent the accumulation of variable length data in it. We 
also recreate the
> The partial tuple isn't re-created here
Done


http://gerrit.cloudera.org:8080/#/c/7639/3/be/src/exec/hdfs-text-scanner.h
File be/src/exec/hdfs-text-scanner.h:

Line 186:   /// Utility function to write out 'num_fields' to 'tuple_'.  This 
is used to parse
> Comment is weird, I suggest rephrasing to something like:
Done


Line 227:   /// Mem pool for the partial tuple.
> Mention that it does not hold tuple data of returned batches because the da
Done


Line 230:   /// Memory to store partial tuples split across buffers.  Memory 
comes from
> Mention that this tuple is always deep copied into the output batch
Done


Line 231:   /// partial_tuple_pool_.  There is only one tuple allocated for 
this object and reused
> The 2nd sentence does not seem to apply anymore because we realloc the part
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5776: Write partial tuple to the correct mempool

2017-08-14 Thread Taras Bobrovytsky (Code Review)
Taras Bobrovytsky has uploaded a new patch set (#4).

Change subject: IMPALA-5776: Write partial tuple to the correct mempool
..

IMPALA-5776: Write partial tuple to the correct mempool

In the text scanner, we were writing the partial tuple variable length
data to data_buffer_pool_ mempool which caused strange behavior, such
as incorrect results.

If we are scanning compressed data, the pool gets attached to the row
batch at the end of a GetNext() call and gets freed before the next
GetNext() call. This is wrong because we expect the data in the partial
tuple to survive between the GetNext() calls. If we are scanning non
compressed data, data_buffer_pool_ never gets cleared and grows over
time until the scanner finishes reading the scan range.

We fix the problem by creating a new memory pool for the partial tuple
will contains partial tuple contant length and variable length data.

Testing:
- Updated test_scanners_fuzz.py to make slightly more significant
  changes to the data files.
- Ran scanner tests locally on ASAN build.
- No new tests were added, because it is difficult to construct test
  cases due to the issue being non-deterministic.

Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
---
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M tests/query_test/test_scanners_fuzz.py
3 files changed, 46 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Henry Robinson (Code Review)
Hello Sailesh Mukil, Dan Hecht,

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

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

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

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..

IMPALA-5743: Support TLS version configuration for Thrift servers

* Add --ssl_minimum_version which controls the minimum SSL/TLS version
  that clients and servers will use when negotiating a secure
  connection.
* Two kinds of version specification are allowed: 'TLSv1.1' enables
  TLSv1.1 and all subsequent verisons. 'TLSv1.1_only' enables only
  TLSv1.1. The latter is not exposed in user-facing text as it is
  typically only used for testing.
* Handle case where platform may not support TLSv1.1 or v1.2 by checking
  OpenSSL version number.
* Bump Thrift toolchain version to -p10.

Testing:
* New tests in thrift-server-test.cc. In particular, test all 36
  configurations of client and server protocol versions, and ensure that
  the expected successes or failures are seen.

Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
---
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-client.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M bin/impala-config.sh
10 files changed, 212 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

2017-08-14 Thread Jim Apple (Code Review)
Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 17:

> That build included this patch. Or do you have reason to believe it
 > didn't?

Ah, OK. That's a part of Gerrit I have not yet understood; thanks for the 
clarification!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 17:

That build included this patch. Or do you have reason to believe it didn't?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

2017-08-14 Thread Jim Apple (Code Review)
Jim Apple has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 17:

> Verified by build: https://jenkins.impala.io/job/gerrit-verify-dryrun/1051/

I'm confused. That build verified https://gerrit.cloudera.org/#/c/5717/19

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


IMPALA-4669: [SECURITY] Add security library to build

* Minor compilation fix
* Add krb5 as a non-toolchain dependency
* Handle legacy versions of libkrb5.so by providing implementation of
  krb5_is_config_principal().
* Link against openssl from the toolchain if 1.0.0 or higher not found
  on build machine.
* Update LICENSE.txt and NOTICE.txt re: OpenSSL code in x509_check_host.{h,c}.

Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Reviewed-on: http://gerrit.cloudera.org:8080/5717
Reviewed-by: Henry Robinson 
Tested-by: Henry Robinson 
---
M CMakeLists.txt
M NOTICE.txt
M be/CMakeLists.txt
M be/src/common/config.h.in
M be/src/kudu/security/init.cc
M be/src/kudu/security/token_verifier.cc
M bin/bootstrap_build.sh
M cmake_modules/FindKerberos.cmake
8 files changed, 92 insertions(+), 18 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 21
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 20: Code-Review+2 Verified+1

Rebase, carry +1 and +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


Patch Set 16: Code-Review+2 Verified+1

Verified by build: https://jenkins.impala.io/job/gerrit-verify-dryrun/1051/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-4669: [SECURITY] Import Kudu security library from 
kudu@314c9d8
..


IMPALA-4669: [SECURITY] Import Kudu security library from kudu@314c9d8

The security library provides Kerberos and TLS facilities to the rpc library.

Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Reviewed-on: http://gerrit.cloudera.org:8080/5716
Reviewed-by: Henry Robinson 
Tested-by: Henry Robinson 
---
M LICENSE.txt
A be/src/kudu/security/CMakeLists.txt
A be/src/kudu/security/ca/cert_management-test.cc
A be/src/kudu/security/ca/cert_management.cc
A be/src/kudu/security/ca/cert_management.h
A be/src/kudu/security/cert-test.cc
A be/src/kudu/security/cert.cc
A be/src/kudu/security/cert.h
A be/src/kudu/security/crypto-test.cc
A be/src/kudu/security/crypto.cc
A be/src/kudu/security/crypto.h
A be/src/kudu/security/init.cc
A be/src/kudu/security/init.h
A be/src/kudu/security/kerberos_util.cc
A be/src/kudu/security/kerberos_util.h
A be/src/kudu/security/krb5_realm_override.cc
A be/src/kudu/security/openssl_util.cc
A be/src/kudu/security/openssl_util.h
A be/src/kudu/security/openssl_util_bio.h
A be/src/kudu/security/security-test-util.cc
A be/src/kudu/security/security-test-util.h
A be/src/kudu/security/simple_acl.cc
A be/src/kudu/security/simple_acl.h
A be/src/kudu/security/test/mini_kdc-test.cc
A be/src/kudu/security/test/mini_kdc.cc
A be/src/kudu/security/test/mini_kdc.h
A be/src/kudu/security/test/test_certs.cc
A be/src/kudu/security/test/test_certs.h
A be/src/kudu/security/test/test_pass.cc
A be/src/kudu/security/test/test_pass.h
A be/src/kudu/security/tls_context.cc
A be/src/kudu/security/tls_context.h
A be/src/kudu/security/tls_handshake-test.cc
A be/src/kudu/security/tls_handshake.cc
A be/src/kudu/security/tls_handshake.h
A be/src/kudu/security/tls_socket.cc
A be/src/kudu/security/tls_socket.h
A be/src/kudu/security/token-test.cc
A be/src/kudu/security/token.proto
A be/src/kudu/security/token_signer.cc
A be/src/kudu/security/token_signer.h
A be/src/kudu/security/token_signing_key.cc
A be/src/kudu/security/token_signing_key.h
A be/src/kudu/security/token_verifier.cc
A be/src/kudu/security/token_verifier.h
A be/src/kudu/security/x509_check_host.cc
A be/src/kudu/security/x509_check_host.h
M bin/rat_exclude_files.txt
A cmake_modules/FindKerberos.cmake
49 files changed, 9,175 insertions(+), 0 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I76daeead00f672aa468f5ab6de4d70eac2078cb2
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 19: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-5477: Fix minidump-2-core tool in breakpad

2017-08-14 Thread Lars Volker (Code Review)
Lars Volker has posted comments on this change.

Change subject: IMPALA-5477: Fix minidump-2-core tool in breakpad
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iafe8cb59316bbec0a2c829f278a23a48238ad6e2
Gerrit-PatchSet: 3
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-5477: Fix minidump-2-core tool in breakpad

2017-08-14 Thread Lars Volker (Code Review)
Lars Volker has submitted this change and it was merged.

Change subject: IMPALA-5477: Fix minidump-2-core tool in breakpad
..


IMPALA-5477: Fix minidump-2-core tool in breakpad

The recent PPC port broke the minidump-2-core tool in the toolchain.
This change undoes a seemingly erroneous #ifdef to fix the tool on
non-PPC systems.

To test this, I built a toolchain locally and made sure that cores
created from minidumps with the tool can be opened by GDB.

I didn't test the change on PPC.

Change-Id: Iafe8cb59316bbec0a2c829f278a23a48238ad6e2
---
M buildall.sh
A 
source/breakpad/breakpad-1b704857f1e78a864e6942e613457e55f1aecb60-patches/0003-fix-md2core.patch
2 files changed, 20 insertions(+), 1 deletion(-)

Approvals:
  Lars Volker: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iafe8cb59316bbec0a2c829f278a23a48238ad6e2
Gerrit-PatchSet: 3
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 


[native-toolchain-CR] IMPALA-5477: Fix minidump-2-core tool in breakpad

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5477: Fix minidump-2-core tool in breakpad
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7658/2/buildall.sh
File buildall.sh:

Line 273: BREAKPAD_VERSION=1b704857f1e78a864e6942e613457e55f1aecb60-p3 
$SOURCE_DIR/source/breakpad/build.sh
Can you move the old version into BUILD_HISTORICAL for consistency with 
elsewhere? 

It's not super-important but good to be consistent.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iafe8cb59316bbec0a2c829f278a23a48238ad6e2
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] [DOCS] Fold some lines

2017-08-14 Thread Laurel Hale (Code Review)
Laurel Hale has posted comments on this change.

Change subject: [DOCS] Fold some  lines
..


Patch Set 1: Code-Review+1

PDF build looks good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I563a71ab9e59c691264c1f6a088e71f4722d3097
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Laurel Hale 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5769: Add periodic minidump cleanup

2017-08-14 Thread Lars Volker (Code Review)
Lars Volker has posted comments on this change.

Change subject: IMPALA-5769: Add periodic minidump cleanup
..


Patch Set 1:

(1 comment)

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

PS1, Line 113: CheckAndRemoveMinidumps
> nit: Maybe we should call this "CheckAndRotateMinidumps" to be consistent w
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie02ff2271412d814f84a4ff42ccbca51d91bf980
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5769: Add periodic minidump cleanup

2017-08-14 Thread Lars Volker (Code Review)
Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-5769: Add periodic minidump cleanup
..

IMPALA-5769: Add periodic minidump cleanup

Minidumps can be written by sending SIGUSR1 to our daemon processes.
That way, an arbitrary number of minidump files can be created. This
change adds minidump cleanup to the periodic log file cleanup to
effectively bound the maximum number of minidumps we keep around.

Change-Id: Ie02ff2271412d814f84a4ff42ccbca51d91bf980
---
M be/src/common/init.cc
M be/src/util/minidump.cc
M be/src/util/minidump.h
M tests/custom_cluster/test_breakpad.py
4 files changed, 47 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie02ff2271412d814f84a4ff42ccbca51d91bf980
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Bharath Vissapragada (Code Review)
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] Fix link to Hadoop ADLS page

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: Fix link to Hadoop ADLS page
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bcbf31735f398a879595846aeb1a6e1c7def9a2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-1478: Improve error message when subquery is used in the ON clause

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-1478: Improve error message when subquery is used in the 
 ON clause
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0d1dc47987de7ea04402e1ead31d81cddf2f96f2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No


[Impala-ASF-CR] Fix link to Hadoop ADLS page

2017-08-14 Thread Laurel Hale (Code Review)
Laurel Hale has posted comments on this change.

Change subject: Fix link to Hadoop ADLS page
..


Patch Set 1: Code-Review+1

I built the documentation and the link works perfectly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bcbf31735f398a879595846aeb1a6e1c7def9a2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-1478: Improve error message when subquery is used in the ON clause

2017-08-14 Thread Bharath Vissapragada (Code Review)
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-1478: Improve error message when subquery is used in the 
 ON clause
..


Patch Set 5: Code-Review+2

+2'ing since the patch is simple.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0d1dc47987de7ea04402e1ead31d81cddf2f96f2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..

IMPALA-4786: Clean up how ImpalaServers are created

ImpalaServer had to be created via an awkward CreateImpalaServer()
method that took a lot of arguments. This patch refactors that code (in
anticipation of some KRPC changes) to follow a more normal lifecycle:

1. ImpalaServer* server = new ImpalaServer(ExecEnv*)
2. RETURN_IF_ERROR(server->Init()) // for error-returning init operations
3. RETURN_IF_ERROR(server->Start())
4. server->Join()

Also add ExecEnv::Init(), and move calls to ExecEnv::StartServices() to
ImpalaServer::StartServices(). This captures a dependency that KRPC will
rely on - where initialization of both ExecEnv and ImpalaServer need to
happen before services are started.

This sets up a clean-up of InProcessImpalaServer, which is too heavy for
the work that it does. That work is deferred to a follow-on patch.

Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
---
M be/src/exprs/expr-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/impalad-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/hdfs-util-test.cc
10 files changed, 108 insertions(+), 137 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 


[Impala-ASF-CR] IMPALA-5769: Add periodic minidump cleanup

2017-08-14 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5769: Add periodic minidump cleanup
..


Patch Set 1: Code-Review+1

(1 comment)

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

PS1, Line 113: CheckAndRemoveMinidumps
nit: Maybe we should call this "CheckAndRotateMinidumps" to be consistent with 
the other log rotation functions?

Your call though, I'd be fine with this name too.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie02ff2271412d814f84a4ff42ccbca51d91bf980
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..


Patch Set 2:

(1 comment)

Fix long line and fix a backend test I had missed.

http://gerrit.cloudera.org:8080/#/c/7597/2/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

Line 218:<< 
state->instance_mem_tracker()->LogUsage(MemTracker::UNLIMITED_DEPTH);
> Long line
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Joe McDonnell (Code Review)
Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..

IMPALA-5598: Fix excessive dumping in MemLimitExceeded

ExecQueryFInstances RPC timeouts in stress tests were
traced to excessive dumping in MemLimitExceeded() and
LogUsage(). The QueryState::Init() hits the process
memory limit, and this causes MemLimitExceeded to dump
the process memory tracker. On the stress test, this
involves dumping hundreds of queries and all the
structures underneath. This is expensive and the
contention between threads accessing these structures
causes RPC timeouts.

This adds the ability to the limit the recursive depth
when dumping memory trackers. It also modifies the
dumping in MemLimitExceeded() to have the following
semantics:
1. The query memory tracker is always logged in full
if it exists.
2. The process memory tracker is logged if the query
memory tracker doesn't exist or if the process memory
limit is being hit. The process memory tracker is
limited to dumping only two levels of children.

Other uses of LogUsage() are not limited (e.g. /memz
and the query memory page on the web UI).

A stress test run with the process memory tracker
limited to two levels showed no RPC timeouts.

Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
---
M be/src/exec/exec-node.cc
M be/src/exec/hash-table-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/initial-reservations.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/runtime-state.cc
M be/src/service/impala-http-handler.cc
M be/src/util/default-path-handlers.cc
9 files changed, 62 insertions(+), 31 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] Fix link to Hadoop ADLS page

2017-08-14 Thread John Russell (Code Review)
John Russell has uploaded a new change for review.

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

Change subject: Fix link to Hadoop ADLS page
..

Fix link to Hadoop ADLS page

At the time I added the original link, the
URL with /current2/ was correct. Now /current2/
no longer exists but /current3/ does. /current/
appears to be a stable alias that points to the
most recent /currentN/ directory so I used that
in the corrected link.

Change-Id: I5bcbf31735f398a879595846aeb1a6e1c7def9a2
---
M docs/topics/impala_adls.xml
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5bcbf31735f398a879595846aeb1a6e1c7def9a2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 


[Impala-ASF-CR] IMPALA-4861: READ WRITE warning on CREATE TABLE LIKE PARQUET.

2017-08-14 Thread Pranay Singh (Code Review)
Pranay Singh has uploaded a new change for review.

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

Change subject: IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.
..

IMPALA-4861: READ_WRITE warning on CREATE TABLE LIKE PARQUET.

Fix: If the source URI does not have write permissions we get
 a "READ WRITE warning" on CREATE TABLE LIKE PARQUET.
 During analyze() in the class CreateTableLikeFileStmt we should
 be checking only for "READ" permission for the source URI
 not for READ and WRITE as the source URI should be readable.

Change-Id: I56799c4da482fb634ce440f8764dd44234dc22ab
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
1 file changed, 1 insertion(+), 1 deletion(-)


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

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


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 10:

This failed in 
authorization/test_authorization.py::TestAuthorization::()::test_impersonation

I don't think we've seen a failure there before so it may be related to the 
change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1050/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..


Patch Set 2: Code-Review+1

(1 comment)

Will let MJ take a look too.

http://gerrit.cloudera.org:8080/#/c/7597/2/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

Line 218:<< 
state->instance_mem_tracker()->LogUsage(MemTracker::UNLIMITED_DEPTH);
Long line


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Joe McDonnell (Code Review)
Joe McDonnell has uploaded a new patch set (#2).

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..

IMPALA-5598: Fix excessive dumping in MemLimitExceeded

ExecQueryFInstances RPC timeouts in stress tests were
traced to excessive dumping in MemLimitExceeded() and
LogUsage(). The QueryState::Init() hits the process
memory limit, and this causes MemLimitExceeded to dump
the process memory tracker. On the stress test, this
involves dumping hundreds of queries and all the
structures underneath. This is expensive and the
contention between threads accessing these structures
causes RPC timeouts.

This adds the ability to the limit the recursive depth
when dumping memory trackers. It also modifies the
dumping in MemLimitExceeded() to have the following
semantics:
1. The query memory tracker is always logged in full
if it exists.
2. The process memory tracker is logged if the query
memory tracker doesn't exist or if the process memory
limit is being hit. The process memory tracker is
limited to dumping only two levels of children.

Other uses of LogUsage() are not limited (e.g. /memz
and the query memory page on the web UI).

A stress test run with the process memory tracker
limited to two levels showed no RPC timeouts.

Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
---
M be/src/exec/exec-node.cc
M be/src/exec/hash-table-test.cc
M be/src/runtime/initial-reservations.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/runtime-state.cc
M be/src/service/impala-http-handler.cc
M be/src/util/default-path-handlers.cc
8 files changed, 61 insertions(+), 30 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5598: Fix excessive dumping in MemLimitExceeded

2017-08-14 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5598: Fix excessive dumping in MemLimitExceeded
..


Patch Set 1:

(3 comments)

Rebased all the way.

http://gerrit.cloudera.org:8080/#/c/7597/1/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

PS1, Line 341: (process_capacity < failed_allocation_size)
> nit: unnecessary parens.
Done


PS1, Line 344: 1
> +1 I was wondering about this as well. The pool tracker is probably not goi
Changed this to two levels. Stress tests show that this isn't a problem. I also 
added constants for the code to use.


http://gerrit.cloudera.org:8080/#/c/7597/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

PS1, Line 324:   int max_recursive_levels = INT_MAX
> Maybe we should make it a required argument to force new callsites to think
Changed this to a required argument.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib151c3894d4c43b508779e6809d77e2f8af89cf2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5787: Dropped status in KuduTableSink::Send()

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5787: Dropped status in KuduTableSink::Send()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7667/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS1, Line 241:   // This can only fail if we set a col to an incorrect 
type, which would be a bug in
 :   // planning, so we can DCHECK.
> can you check with the kudu developers (e.g. on the Kudu dev slack channel)
I checked with the Kudu team, and as long as the schema is what we want when 
the table is opened, it should be fine. If the schema changes in an 
incompatible way after the table is opened but before the writes are applied, 
the writes will fail gracefully at Appy().

However, there's a window of vulnerability between when we check the schema in 
the FE and when the table object is opened, so I explored this in detail, and 
there are a few cases to consider:
  
- Column type changes: we don't allow altering existing column's types.
- Column renamed: everything works as expected.
- Column added: we don't allow adding primary keys, and non-nullable columns 
that are added must have a default, so everything works as expected.
- Column dropped: this causes a crash, with or without this change. I filed 
IMPALA-5799


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia228d98ca90de8638b0afcc242c788e225fb450f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4794: Grouping distinct agg plan robust to data skew

2017-08-14 Thread Tianyi Wang (Code Review)
Tianyi Wang has posted comments on this change.

Change subject: IMPALA-4794: Grouping distinct agg plan robust to data skew
..


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7643/3//COMMIT_MSG
Commit Message:

Line 11: plan partitions data between phase-1 and phase-2 by the grouping exprs.
> the grouping exprs
Done


Line 12: Under this strategy the data skewness on the grouping exprs directly
> make this statement about skew a separate sentence
Done


Line 13: impacts performance. The new plan partitions data by both the grouping
> by both the grouping and distinct agg exprs
Done


Line 14: exprs and distinct agg exprs, then adds one more aggregation and
> Try to avoid descriptions like "supposed to be". We should test and underst
Done


Line 19: sufficient coverage. The pattern is that the distinct agg exprs are
> the first exchange node
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4794: Grouping distinct agg plan robust to data skew

2017-08-14 Thread Tianyi Wang (Code Review)
Tianyi Wang has uploaded a new patch set (#4).

Change subject: IMPALA-4794: Grouping distinct agg plan robust to data skew
..

IMPALA-4794: Grouping distinct agg plan robust to data skew

This patch changes the query plan for grouping distinct aggregations to
be more robust to data skew in the grouping expressions. The existing
plan partitions data between phase-1 and phase-2 by the grouping exprs.
Under this strategy the data skewness on the grouping exprs directly
impacts performance. The new plan partitions data by both the grouping
exprs and distinct agg exprs, then adds one more aggregation and
exchange node. The new plan is more robust to data skew but does more
work than the old plan.

Testing: Modified existing planner tests which already provide
sufficient coverage. The pattern is that the distinct agg exprs are
added to the first exchange node, followed by an additional merge agg
and exchange node.

Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
---
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/distinct.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
7 files changed, 215 insertions(+), 131 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..

IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

Plumbing statuses through TextConverter::CodegenWriteSlot(), which
eventually propagate to the runtime profile.

Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
---
M be/src/exec/hdfs-scanner.cc
M be/src/exec/text-converter.cc
M be/src/exec/text-converter.h
3 files changed, 53 insertions(+), 41 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 


[Impala-ASF-CR] IMPALA-2689: Log why codegen is disabled in TextConverter::CodegenWriteSlot()

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2689: Log why codegen is disabled in 
TextConverter::CodegenWriteSlot()
..


Patch Set 3: Code-Review+2

One business day has elapsed and it looks like Lars' comments were addressed. 
Let's move forward.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I355efe2d5566ba6a1b72648763794a79c80c3089
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers

2017-08-14 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift 
servers
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2422: Fix escaping in LIKE clause

2017-08-14 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2422: Fix escaping in LIKE clause
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7660/2/be/src/exprs/like-predicate.cc
File be/src/exprs/like-predicate.cc:

Line 34: // A regex to match any regex pattern is equivalent to a substring 
search.
isn't this comment referring to line 38? assuming so, it should be kept 
adjacent it it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec0f5d99c855ea8d4fe1bcf28c05780ba315034b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4794: Grouping distinct agg plan robust to data skew

2017-08-14 Thread Tianyi Wang (Code Review)
Tianyi Wang has posted comments on this change.

Change subject: IMPALA-4794: Grouping distinct agg plan robust to data skew
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7643/2/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

Line 859:* as its root.
> update comment
Done


Line 864: // The phase-1 aggregation node is already in the child fragment.
> move into previous line
Done


Line 916: phase2AggNode.setIntermediateTuple();
> Newlines should be used to visually cluster code that logically belongs tog
Done


Line 917: 
> You still need to do phase2AggNode.setIsPreagg(ctx_);
I think that only works with grouping expr and is at line 922.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4794: Grouping distinct agg plan robust to data skew

2017-08-14 Thread Tianyi Wang (Code Review)
Tianyi Wang has uploaded a new patch set (#3).

Change subject: IMPALA-4794: Grouping distinct agg plan robust to data skew
..

IMPALA-4794: Grouping distinct agg plan robust to data skew

This patch changes the query plan for grouping distinct aggregations to
be more robust to data skew in the grouping expressions. The existing
plan partitions data between phase-1 and phase-2 by grouping expr and
the data skewness on grouping expr directly impacts performance. The
new plan partitions data by both grouping expr and distinct aggregation
expr, then adds one more aggregation and exchange node. It is supposed
to be faster with data skew but slower otherwise.

Teting: Modified existing planner tests which already provide sufficient
coverage. The pattern is that the distinct aggregation expr is added to
exchange node, followed by an additional merge aggregation and exchange
node.

Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
---
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/distinct.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
7 files changed, 215 insertions(+), 131 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 


[Impala-ASF-CR] IMPALA-4794: Grouping distinct agg plan robust to data skew

2017-08-14 Thread Alex Behm (Code Review)
Alex Behm has posted comments on this change.

Change subject: IMPALA-4794: Grouping distinct agg plan robust to data skew
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7643/2/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

Line 859:* as its root and 'node' is the second phase of the distinct 
aggregation.
update comment


Line 864:   throws ImpalaException {
move into previous line


Line 916: 
Newlines should be used to visually cluster code that logically belongs 
together. Move this newline right before "DataPartition mergePartition;" - 
that's where we begin to add the merge agg node (in the preceding code 
separated by newline we added the phase2AggNode as an intermediate aggregation 
step)


Line 917: phase2AggNode.unsetNeedsFinalize();
You still need to do phase2AggNode.setIsPreagg(ctx_);


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bdada0e328b555900c7b7ff8aabc8eb15ae8fa9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5787: Dropped status in KuduTableSink::Send()

2017-08-14 Thread Matthew Jacobs (Code Review)
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5787: Dropped status in KuduTableSink::Send()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7667/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS1, Line 241:   // This can only fail if we set a col to an incorrect 
type, which would be a bug in
 :   // planning, so we can DCHECK.
can you check with the kudu developers (e.g. on the Kudu dev slack channel) 
that this is also safe in the context of concurrent schema changes? I'm 95% 
confident that this is the case, but it'd be good to check.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia228d98ca90de8638b0afcc242c788e225fb450f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..


Patch Set 19:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4669: [SECURITY] Add security library to build

2017-08-14 Thread Henry Robinson (Code Review)
Hello Impala Public Jenkins, Michael Ho,

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

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

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

Change subject: IMPALA-4669: [SECURITY] Add security library to build
..

IMPALA-4669: [SECURITY] Add security library to build

* Minor compilation fix
* Add krb5 as a non-toolchain dependency
* Handle legacy versions of libkrb5.so by providing implementation of
  krb5_is_config_principal().
* Link against openssl from the toolchain if 1.0.0 or higher not found
  on build machine.
* Update LICENSE.txt and NOTICE.txt re: OpenSSL code in x509_check_host.{h,c}.

Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
---
M CMakeLists.txt
M NOTICE.txt
M be/CMakeLists.txt
M be/src/common/config.h.in
M be/src/kudu/security/init.cc
M be/src/kudu/security/token_verifier.cc
M bin/bootstrap_build.sh
M cmake_modules/FindKerberos.cmake
8 files changed, 92 insertions(+), 18 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f327810066bee7f3ac107b0295480fb9ed45e14
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 


[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..


Patch Set 12: Code-Review+1

Carry +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3548: Prune runtime filters based on query options in 
the FE
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5787: Dropped status in KuduTableSink::Send()

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: IMPALA-5787: Dropped status in KuduTableSink::Send()
..

IMPALA-5787: Dropped status in KuduTableSink::Send()

KuduTableSink drops the Status returned from WriteKuduValue(),
potentially hiding failures.

There are only two possible ways WriteKuduValue() can fail: one of the
KuduPartialRow::Set functions fails, which can only happen if
the specified type is incorrect, which would be a bug in planning and
is therefore DCHECK-able, or if TimestampValue::UtcToUnixTimeMicros()
fails, which WriteKuduValue() DCHECKs against but also has a path to
return the error in release builds.

So similarly, this patch adds a DCHECK that the status is OK, and a
path to return the error in release builds.

It also adds WARN_UNUSED_RESULT to WriteKuduValue() to prevent similar
bugs in the future.

Testing:
- Ran the Kudu tests.

Change-Id: Ia228d98ca90de8638b0afcc242c788e225fb450f
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-util.h
2 files changed, 8 insertions(+), 2 deletions(-)


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

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


[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 5:

(9 comments)

This looks good! Just needs to merge with the recent ThriftServer changes, and 
consider adding a test where you set the number of concurrent clients and 
confirm that only that many can connect at once.

http://gerrit.cloudera.org:8080/#/c/7061/5/be/src/benchmarks/network-perf-benchmark.cc
File be/src/benchmarks/network-perf-benchmark.cc:

Line 206:   impala::InitCommonRuntime(argc, argv, false, 
impala::TestInfo::BE_TEST);
> I don't think network-perf-benchmark gets used much because it had bit rott
This is fine - it's sufficient to keep it compiling for now.


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

Line 91:   ThriftServer* server = new ThriftServer("CatalogService", processor,
As you'll have seen, IMPALA-5696 switched over to a new ThriftServerBuilder 
idiom for constructing ThriftServer objects. I think what you'll need to do is 
remove the threaded() and thread_pool() methods, and just add a 
max_concurrent_cnxns(int) method.


http://gerrit.cloudera.org:8080/#/c/7061/5/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

Line 61:   int32_t maxTasks,
could you add a TODO to this class to suggest we determine which of these 
c'tors are actually needed?


PS5, Line 114: notifies
notified?


Line 117:   // The maximum number of running tasks allowed at a time.
nit: put a new line before this one.


http://gerrit.cloudera.org:8080/#/c/7061/5/be/src/rpc/thrift-server.cc
File be/src/rpc/thrift-server.cc:

PS5, Line 409: stringstream key_prefix_ss;
 : key_prefix_ss << "impala.thrift-server." << name_;
while you're here use Substitute("impala.thrift-server.$0", name_) ?


http://gerrit.cloudera.org:8080/#/c/7061/5/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

PS5, Line 176: conncurrent
typo


PS5, Line 177: until a thread becomes available
nit: this makes it sound like the threads are pooled. Suggest 'connections will 
block until fewer than num_worker_threads_ threads are concurrently active' or 
something.


http://gerrit.cloudera.org:8080/#/c/7061/5/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:

PS5, Line 144: TThreadedServer and
 :   // TThreadPoolServer
remove these two?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5708: Test failure with invalid exec summary

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5708: Test failure with invalid exec summary
..


Patch Set 3: Code-Review+2

Carrying forward

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id52ac62da2b01f9e163e97cbe4590f8db6b663d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5708: Test failure with invalid exec summary

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5708: Test failure with invalid exec summary
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id52ac62da2b01f9e163e97cbe4590f8db6b663d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

2017-08-14 Thread Jim Apple (Code Review)
Jim Apple has posted comments on this change.

Change subject: IMPALA-5764: Allow overriding packaged components
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export 
DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
> Technically this "breaks" the C6 build, but because the cdh6.x version of I
This is the code review tool for Apache Impala. It is fine to add features to 
Impala to help other software that works with Impala, including proprietary 
products.

However, please try to explain enough in your discussion here so that a person 
who does not work at Cloudera can help review your code and understand all the 
jargon like "pushed from CDH5" and "cauldron".


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5776: Write partial tuple to the correct mempool

2017-08-14 Thread Alex Behm (Code Review)
Alex Behm has posted comments on this change.

Change subject: IMPALA-5776: Write partial tuple to the correct mempool
..


Patch Set 3:

(7 comments)

I like the new solution, it's easier to understand.

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

Line 269:   while (true) {
> It's used on line 277. Are you suggesting to remove it from the signature o
Oh I missed that sorry. Ok to leave.


http://gerrit.cloudera.org:8080/#/c/7639/3/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

Line 804:   // Copy over all materialized slots in the partial tuple.
Only describes what the code does. How about something like:

// Deep copy to simplify memory ownership.


Line 808:   // prevent the accumulation of variable length data in it. We 
also recreate the
The partial tuple isn't re-created here


http://gerrit.cloudera.org:8080/#/c/7639/3/be/src/exec/hdfs-text-scanner.h
File be/src/exec/hdfs-text-scanner.h:

Line 186:   /// Utility function to write out 'num_fields' to 'tuple_'.  This 
is used to parse
Comment is weird, I suggest rephrasing to something like:

Utility function to parse 'num_fields' and materialize the resulting slots into 
'partial_tuple_'. The data of var-len fields is copied into 
'partial_tuple_pool_'.


Line 227:   /// Mem pool for the partial tuple.
Mention that it does not hold tuple data of returned batches because the data 
is always deep-copied into the output batch


Line 230:   /// Memory to store partial tuples split across buffers.  Memory 
comes from
Mention that this tuple is always deep copied into the output batch


Line 231:   /// partial_tuple_pool_.  There is only one tuple allocated for 
this object and reused
The 2nd sentence does not seem to apply anymore because we realloc the 
partial_tuple_


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60ba5c113aefd17f697c1888fd46a237ef396540
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5764: Allow overriding packaged components

2017-08-14 Thread Zach Amsden (Code Review)
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5764: Allow overriding packaged components
..


Patch Set 2:

(4 comments)

A couple of minor nits.  Also, let me make sure I didn't accidentally introduce 
typos during edits and that the script and data load still work.

http://gerrit.cloudera.org:8080/#/c/7581/2/README.md
File README.md:

Line 69: | PACKAGED_COMPONENTS_NAME | "cdh" | Identifier used to uniquify paths 
for potentially incompatible component builds. |
This doesn't currently match the "cdh5" label used in the shell script.


http://gerrit.cloudera.org:8080/#/c/7581/2/bin/impala-config.sh
File bin/impala-config.sh:

Line 180: export 
DOWNLOAD_PACKAGED_COMPONENTS=${DOWNLOAD_PACKAGED_COMPONENTS-"$NO_THIRDPARTY"}
Technically this "breaks" the C6 build, but because the cdh6.x version of 
Impala is pushed right now from CDH5 with a thirdparty directory, this is still 
working.  Will need to update the cauldron build to fix this.


Line 361:   export 
PACKAGED_COMPONENTS_HOME="$IMPALA_TOOLCHAIN/$PACKAGED_COMPONENTS_NAME"
This changes the download directory for the CDH pieces of the toolchain; I 
presume that is okay to do, might result in duplicate copies but nothing should 
break because of this.


Line 382: export 
MINI_DFS_BASE_DATA_DIR="$IMPALA_HOME/${PACKAGED_COMPONENTS_NAME}-hdfs-data"
This forces a data reload - unless there is any objection, I'm not going to try 
to change that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c2662e6f62adc924cc5de7a371202126046545
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Tianyi Wang (Code Review)
Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 10:

> (2 comments)
 > 
 > Looks good. If you fix the last couple of nits and rebase I can
 > start the merge.

Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 10:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Tianyi Wang (Code Review)
Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..

IMPALA-5327: Handle return of JNI GetStringUTFChar

GetStringUTFChars may return NULL or throw exception and it is not
handled in two places. This patch checks the return value/exception,
logs if there is error and return error to the caller. A helper class
JniUtfCharGuard is added to jni-util.h/cc to manage the lifetime.

Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
---
M be/src/common/status.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
4 files changed, 68 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-5327: Handle return of JNI GetStringUTFChar

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar
..


Patch Set 8: Code-Review+2

(2 comments)

Looks good. If you fix the last couple of nits and rebase I can start the merge.

http://gerrit.cloudera.org:8080/#/c/7642/8/be/src/util/jni-util.h
File be/src/util/jni-util.h:

Line 164:   DISALLOW_COPY_AND_ASSIGN(JniUtfCharGuard);
Nit: move it inside 'private'.


http://gerrit.cloudera.org:8080/#/c/7642/8/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 53: str = "";
Nit: seems simpler to just initialise it to point to the empty string constant 
like it was before.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5796: CTAS for Kudu fails with expr rewrite

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: IMPALA-5796: CTAS for Kudu fails with expr rewrite
..

IMPALA-5796: CTAS for Kudu fails with expr rewrite

When an expr rewrite occurs, we reanalyze the statement. Some state
that is set in TableDef::analyze() wasn't being reset() first, causing
a failure during reanalysis.

This patch adds TableDef::reset(), which clears the TableDef state
that is set during analyze().

Testing:
- Added a regression test in AnalyzeDDLTest

Change-Id: Ia67bb33736b5a843663b226cdd0fa5bd839cbea1
---
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/analysis/TableDef.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
4 files changed, 32 insertions(+), 5 deletions(-)


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

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


[Impala-ASF-CR] IMPALA-2615: support [[nodiscard]] on Status

2017-08-14 Thread Jim Apple (Code Review)
Jim Apple has posted comments on this change.

Change subject: IMPALA-2615: support [[nodiscard]] on Status
..


Patch Set 9: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I972543af2e9f98b12dcbb5479b4c1a7d53952197
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

2017-08-14 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS8, Line 467: // Represent this as an array of bytes.
 :   return ArrayType::get(GetType(TYPE_TINYINT), type.len);
> IMHO, this change in behavior makes GetType() an error prone interface. Giv
I think the relationship is analogous to the existing cases:

  int64  <==> BIGINT
  StringValue<==> STRING
  TimestampValue <==> TIMESTAMP
  int128 <==> DECIMAL(38,7)
  int8[n]<==> FIXED_UDA_INTERMEDIATE[n]


It's already a bug if you call this with TYPE_STRING when constructing an IR 
function argument since it returns StringValue (we should rename 
string_val_type and timestamp_val_type for sure though).


http://gerrit.cloudera.org:8080/#/c/7526/11/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS11, Line 1540: bitcast { i64, i8* }* %dst_lowered_ptr to 
%"struct.impala_udf:
> This seems unnecessary now but I guess DCE will eliminate it.
Yeah, DCE will get it. I think it's simpler than modifying the codegen 
interfaces to avoid emitting the instruction in this special case.


http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/exprs/agg-fn-evaluator.cc
File be/src/exprs/agg-fn-evaluator.cc:

PS8, Line 478: TYPE_FIXED_UDA_INTERMEDIATE:
> Is TYPE_FIXED_UDA_INTERMEIDATE limited to built-in UDA only ?
stddev already uses it internally and there doesn't seem to have been anything 
preventing declaration of a UDA using CHAR(n). Codegen is disabled though. On 
master I can declare a UDA with a CHAR intermediate if I load the UDAs from 
this patch:

  $ MAKE_CMD=ninja ./testdata/bin/copy-udfs-udas.sh -build
  $ git checkout origin/master
  $ git rev-parse HEAD
  df9ecdc45a34b176f2ee6eda8e12d0195fba9ae7


  [localhost:21000] > create aggregate function udasum(int) RETURNS int
INTERMEDIATE CHAR(10) LOCATION '/test-warehouse/libTestUdas.so' 
UPDATE_FN='AggCharIntermediateUpdate'
INIT_FN='AggCharIntermediateInit' MERGE_FN='AggCharIntermediateMerge'
FINALIZE_FN='AggCharIntermediateFinalize';
Query: create aggregate function udasum(int) RETURNS int
INTERMEDIATE CHAR(10) LOCATION '/test-warehouse/libTestUdas.so' 
UPDATE_FN='AggCharIntermediateUpdate'
INIT_FN='AggCharIntermediateInit' MERGE_FN='AggCharIntermediateMerge'
FINALIZE_FN='AggCharIntermediateFinalize'
  Fetched 0 row(s) in 0.17s
  [localhost:21000] > select year, month, day, udasum(int_col), sum(int_col) 
from functional.alltypesagg group by year, month, day;
  Query: select year, month, day, udasum(int_col), sum(int_col) from 
functional.alltypesagg group by year, month, day
  Query submitted at: 2017-08-14 10:54:10 (Coordinator: 
http://tarmstrong-box:25000)
  Query progress can be monitored at: 
http://tarmstrong-box:25000/query_plan?query_id=3546da729d693928:a81fcd1b
  Socket error 104: Connection reset by peer
[Not connected] > Goodbye tarmstrong


So I guess it's allowed but broken.

Arguably it might be better to disallow it instead of fixing it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

2017-08-14 Thread Tim Armstrong (Code Review)
Hello Matthew Jacobs,

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

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

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

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..

IMPALA-3931: arbitrary fixed-size uda intermediate types

Make many builtin aggregate functions use fixed-length intermediate
types:
* avg()
* ndv()
* stddev(), variance(), etc
* distinctpc(), distinctpcsa()

sample(), appx_median(), histogram() and group_concat() actually
allocate var-len data so aren't changed.

This has some major benefits:
* Spill-to-disk works properly with these aggregations.
* Aggregations are more efficient because there is one less pointer
  indirection.
* Aggregations use less memory, because we don't need an extra 12-byte
  StringValue for the indirection.

Adds a special-purpose internal type FIXED_UDA_INTERMEDIATE. The type
is represented in the same way as CHAR - a fixed-size array of bytes,
stored inline in tuples. However, it is not user-visible and does
not support CHAR semantics, i.e. users can't declare tables, functions,
etc with the type. The pointer and length is passed into aggregate functions
wrapped in a StringVal.

Updates some internal codegen functions to work better with the new
type. E.g. store values directly into the result tuple instead of
via an intermediate stack allocation.

Testing:
This change only affects builtin aggregate functions, for which we
have test coverage already. If we were to allow wider use of this type,
it would need further testing.

Added an analyzer test to ensure we can't use the type for UDAs.

Added a regression test for spilling avg().

Added a regression test for UDA with CHAR intermediate hitting DCHECK.

Perf:
Ran TPC-H locally. TPC-H Q17, which has a high-cardinality AVG(),
improved dramatically.

+--+---+-++++
| Workload | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) | 
Delta(GeoMean) |
+--+---+-++++
| TPCH(60) | parquet / none / none | 18.44   | -17.54%| 11.92  | -5.34% 
|
+--+---+-++++

+--+--+---++-++---++-+---+
| Workload | Query| File Format   | Avg(s) | Base Avg(s) | 
Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters |
+--+--+---++-++---++-+---+
| TPCH(60) | TPCH-Q12 | parquet / none / none | 18.40  | 17.64   | +4.32%   
  |   0.77%   |   1.09%| 1   | 5 |
| TPCH(60) | TPCH-Q22 | parquet / none / none | 7.07   | 6.90| +2.36%   
  |   0.28%   |   0.30%| 1   | 5 |
| TPCH(60) | TPCH-Q3  | parquet / none / none | 12.37  | 12.11   | +2.10%   
  |   0.18%   |   0.15%| 1   | 5 |
| TPCH(60) | TPCH-Q7  | parquet / none / none | 42.48  | 42.09   | +0.93%   
  |   2.45%   |   0.80%| 1   | 5 |
| TPCH(60) | TPCH-Q6  | parquet / none / none | 3.18   | 3.15| +0.89%   
  |   0.67%   |   0.76%| 1   | 5 |
| TPCH(60) | TPCH-Q19 | parquet / none / none | 7.24   | 7.20| +0.50%   
  |   0.95%   |   0.67%| 1   | 5 |
| TPCH(60) | TPCH-Q10 | parquet / none / none | 13.37  | 13.30   | +0.50%   
  |   0.48%   |   1.39%| 1   | 5 |
| TPCH(60) | TPCH-Q5  | parquet / none / none | 7.47   | 7.44| +0.36%   
  |   0.58%   |   0.54%| 1   | 5 |
| TPCH(60) | TPCH-Q11 | parquet / none / none | 2.03   | 2.02| +0.06%   
  |   0.26%   |   1.95%| 1   | 5 |
| TPCH(60) | TPCH-Q4  | parquet / none / none | 5.48   | 5.50| -0.27%   
  |   0.62%   |   1.12%| 1   | 5 |
| TPCH(60) | TPCH-Q13 | parquet / none / none | 22.11  | 22.18   | -0.31%   
  |   0.18%   |   0.55%| 1   | 5 |
| TPCH(60) | TPCH-Q15 | parquet / none / none | 8.45   | 8.48| -0.32%   
  |   0.40%   |   0.47%| 1   | 5 |
| TPCH(60) | TPCH-Q9  | parquet / none / none | 33.39  | 33.66   | -0.81%   
  |   0.75%   |   0.59%| 1   | 5 |
| TPCH(60) | TPCH-Q21 | parquet / none / none | 71.34  | 72.07   | -1.01%   
  |   1.84%   |   1.79%| 1   | 5 |
| TPCH(60) | TPCH-Q14 | parquet / none / none | 5.93   | 6.00| -1.07%   
  |   0.15%   |   0.69%| 1   | 5 |
| TPCH(60) | TPCH-Q20 | parquet / none / none | 5.72   | 5.79| -1.09%   
  |   0.59%   |   0.51%| 1   | 5 |
| TPCH(60) | TPCH-Q18 | parquet / none / none | 45.42  | 

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

2017-08-14 Thread Henry Robinson (Code Review)
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 5:

Sorry for the delay, John! I'll try to have comments later today. There's no 
particular etiquette - you're free to post a new patch if the reviewers (i.e. 
me!) are taking their time over comments...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-3931: arbitrary fixed-size uda intermediate types

2017-08-14 Thread Michael Ho (Code Review)
Michael Ho has posted comments on this change.

Change subject: IMPALA-3931: arbitrary fixed-size uda intermediate types
..


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS8, Line 467: // Represent this as an array of bytes.
 :   return ArrayType::get(GetType(TYPE_TINYINT), type.len);
> I think this makes sense so long as we make it clear that the function retu
IMHO, this change in behavior makes GetType() an error prone interface. Given 
there is no clear guideline on which one to use, it's likely that one may call 
this function when generating a signature for a IR function or any call sites 
which use both GetType() and GetUnloweredType() may result in surprises. It 
seems safer to have a separate function for the internal representation.

We should also document clearly how FIXED_UDA_INTERMEDIATE is represented in 
IR. For instance, we are mostly exposing it as a StringVal but when is it 
appropriate to represent it as an ArrayType.


http://gerrit.cloudera.org:8080/#/c/7526/11/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS11, Line 1540: bitcast { i64, i8* }* %dst_lowered_ptr to 
%"struct.impala_udf:
This seems unnecessary now but I guess DCE will eliminate it.


http://gerrit.cloudera.org:8080/#/c/7526/8/be/src/exprs/agg-fn-evaluator.cc
File be/src/exprs/agg-fn-evaluator.cc:

PS8, Line 478: TYPE_FIXED_UDA_INTERMEDIATE:
> A DCHECK doesn't seem appropriate since a UDA could set the incorrect thing
Is TYPE_FIXED_UDA_INTERMEIDATE limited to built-in UDA only ?

Btw, did we not support TYPE_CHAR as intermediate type before ? Is it a change 
in behavior ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

2017-08-14 Thread John Sherman (Code Review)
John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 5:

I don't quite know the correct etiquette, should I fix the merge conflict with 
recent changes or wait for review comments on my current change set?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[native-toolchain-CR] Bump Kudu version to b198ed8

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has submitted this change and it was merged.

Change subject: Bump Kudu version to b198ed8
..


Bump Kudu version to b198ed8

Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 


[native-toolchain-CR] Bump Kudu version to b198ed8

2017-08-14 Thread Matthew Jacobs (Code Review)
Matthew Jacobs has posted comments on this change.

Change subject: Bump Kudu version to b198ed8
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No


[native-toolchain-CR] Bump Kudu version to b198ed8

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has posted comments on this change.

Change subject: Bump Kudu version to b198ed8
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-4737: Prevent SIGUSR1 from killing daemons when minidumps are disabled

2017-08-14 Thread John Sherman (Code Review)
John Sherman has posted comments on this change.

Change subject: IMPALA-4737: Prevent SIGUSR1 from killing daemons when 
minidumps are disabled
..


Patch Set 2:

No, I do not feel strongly about this.

> > Should any startup scripts be modified to trap '' SIGUSR1 before
 > > launching the daemons since there is still technically a race
 > from
 > > daemon startup to when the mini-dump signal handler is
 > registered?
 > > (Though from what I can tell the typical usage of the SIGUSR1
 > > mini-dump handler would very likely never encounter this race, so
 > > maybe not worth fixing?)
 > 
 > Yes, that race exists, I don't think it's worth fixing though.
 > Human operators should not hit it. Automated scripts that regularly
 > trigger minidumps sending SIGUSR1 could hit this, but I think we
 > should not encourage those anyways. Let me know if you feel
 > strongly about this.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I13d866a2eec832500131954a7f693c33585ea51e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[native-toolchain-CR] Bump Kudu version to b198ed8

2017-08-14 Thread Thomas Tauber-Marshall (Code Review)
Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: Bump Kudu version to b198ed8
..

Bump Kudu version to b198ed8

Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
---
M buildall.sh
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia62deb0e61f06de4ce4e95476b85988e1e2754bb
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 


[Impala-ASF-CR] IMPALA-5715: (mitigation only) defer destruction of MemTrackers

2017-08-14 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5715: (mitigation only) defer destruction of MemTrackers
..


Patch Set 6:

> I wrote up a short page on the wiki per Henry's suggestion:
 > https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala

Thanks, that page looks good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No


  1   2   >