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

2017-10-17 Thread Pranay Singh (Code Review) 
Hello Joe McDonnell, Tim Armstrong, Bikramjeet Vig,

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

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

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

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

IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder

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

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

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

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

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


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

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

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

2017-10-17 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8034 )

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


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@348
PS8, Line 348: f the column.
> We should be using Close() here instead of unregistering every tracker (thi
Removed the Close() since I'm using the MemTracker of
scan_node_->mem_tracker so won't be using 1000's of memtrackers now


http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:

http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@133
PS8, Line 133: di
> nit: inline here and nearby isn't necessary for a couple of reason. First,
Removed inline


http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@184
PS8, Line 184: Node(const T& v, const NodeIndex& n) : value(v), next(n) { }
> Nit: check isn't necessary - Release() does this for you.
Removed the check


http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@194
PS8, Line 194: enum { INVALID_INDEX = 4 };
> The TODO should be to use TryConsume(). The asynchronous checks are not the
Changed the TODO to use TryConsume()


http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@251
PS8, Line 251: / This function
> I agree with Joe's comment. All codepaths should call Close(), so we can ju
Added a DCHECK found during the testing that some of the column readers do not 
call Close, so I've added ReleaseBytes() so that accounting of memory used for 
dictionary is release when DictDecoder is destroyed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Number: 8034
Gerrit-PatchSet: 9
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 18 Oct 2017 05:28:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

2017-10-17 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 2:

> (1 comment)

I tried cherry-picking:
https://github.com/apache/kudu/commit/50c7d3249ab5ca19fb4d3c0c8748a4a1c5945a12

But since it touches so many files, it doesn't cherry-pick cleanly and adds 
some extra code from patches that we don't have in our code base, due to how 
the diff was generated.

Also, it incorrectly modifies out .clang-tidy file since Kudu's .clang tidy 
does not exist in our fetch of their code. Also, the 3 way merge gets confused 
and ends up modifying our gutil/bind_internal.h instead of their 
kudu/gutil/bind_internal.h.

We can resolve all this manually, but I think it's a bit too risky.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 18 Oct 2017 04:45:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] Add 'psmisc' to bootstrap system.sh.

2017-10-17 Thread Jim Apple (Code Review) 
Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8306 )

Change subject: Add 'psmisc' to bootstrap_system.sh.
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I378b6fbf7a7b3f0b42ecf064e98cfe6e29b3
Gerrit-Change-Number: 8306
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Wed, 18 Oct 2017 04:44:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8308 )

Change subject: Allow configuration of values passed into kerberos env vars
..


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 18 Oct 2017 04:05:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5019: Decimal V2 addition

2017-10-17 Thread Taras Bobrovytsky (Code Review) 
Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8309 )

Change subject: IMPALA-5019: Decimal V2 addition
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8309/1/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java@268
PS1, Line 268: // Not yet implemented - fall back to V1 rules.
I think this can now be removed now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336
Gerrit-Change-Number: 8309
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 18 Oct 2017 02:30:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8274 )

Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
..


Patch Set 2: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24
Gerrit-Change-Number: 8274
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 18 Oct 2017 02:18:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5019: Decimal V2 addition

2017-10-17 Thread Taras Bobrovytsky (Code Review) 
Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8309


Change subject: IMPALA-5019: Decimal V2 addition
..

IMPALA-5019: Decimal V2 addition

In this patch, we implement the new decimal return type rules for
addition expressions. These rules become active when the query option
DECIMAL_V2 is enabled. The algorithm for determining the type of the
result is described in the JIRA.

DECIMAL V1:
++
| typeof(cast(1 as decimal(38,0)) + cast(0.1 as decimal(38,38))) |
++
| DECIMAL(38,38) |
++

DECIMAL V2:
++
| typeof(cast(1 as decimal(38,0)) + cast(0.1 as decimal(38,38))) |
++
| DECIMAL(38,6)  |
++

This patch required backend changes. We implement an algorithm where
we handle the whole and fractional parts separately, and then combine
them to get the final result. This is more complex and slower. We try
to avoid this by first checking if the result would into int128.

Testing:
- Added expr tests.
- Tested locally on my machine with a script that generates random
  decimal numbers and checks that Impala adds them correctly.

Performance:

For the common case, performance remains the same.
  select cast(2.2 as decimal(18, 1) + cast(2.2 as decimal(18, 1)

  BEFORE: 4.74s
  AFTER:  4.73s

In this case, we check if it is necessary to do the complex addition,
and it turns out to be not necessary. We see a slowdown because the
result needs to be scaled down by dividing.
  select cast(2.2 as decimal(38, 19) + cast(2.2 as decimal(38, 19)

  BEFORE: 1.63s
  AFTER:  13.57s

In following case, we take the most complex path and see the most
signification perfmance hit.
  select cast(7.5 as decimal(38,37)) + cast(2.2 as decimal(38,37))

  BEFORE: 1.63s
  AFTER: 20.57

Change-Id: I401049c56d910eb1546a178c909c923b01239336
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.inline.h
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
3 files changed, 282 insertions(+), 69 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336
Gerrit-Change-Number: 8309
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky

[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker 
errors
..


Patch Set 6: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 18 Oct 2017 01:02:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

2017-10-17 Thread Michael Ho (Code Review) 
Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8308 )

Change subject: Allow configuration of values passed into kerberos env vars
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 18 Oct 2017 00:34:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

2017-10-17 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8308 )

Change subject: Allow configuration of values passed into kerberos env vars
..


Patch Set 1:

> Uploaded patch set 1.

This cherry-pick from Kudu was clean and is required for avoiding code 
divergence with Kudu on IMPALA-5129 (https://gerrit.cloudera.org/#/c/7938/)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 18 Oct 2017 00:01:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

2017-10-17 Thread Sailesh Mukil (Code Review) 
Hello Todd Lipcon,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: Allow configuration of values passed into kerberos env vars
..

Allow configuration of values passed into kerberos env vars

We always used hardcoded constants for the following kerberos
environment variables:

KRB5CCNAME and KRB5RCACHETYPE.

This patch allows for the configuration of these variables by taking
arguments to InitKerberosForServer().

Callsites within Kudu have not been changed as all the parameters have
default values.

The motivation for this patch is that, Impala as a user of the
KuduRPC and Kudu security libraries, needs to have a file based
credential cache since the kinit happens on the C++ side and this cache
needs to be read by the Java side too. Hence, we cannot have it in memory.
Also, Impala still requires replay protection, since some Impala services
use Thrift which lacks the nonce mechanism that KRPC uses for replay
protection.

Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Reviewed-on: http://gerrit.cloudera.org:8080/8247
Reviewed-by: Todd Lipcon 
Tested-by: Todd Lipcon 
---
M be/src/kudu/security/init.cc
M be/src/kudu/security/init.h
2 files changed, 19 insertions(+), 12 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-17 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8305 )

Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/client-request-state.h@341
PS1, Line 341: time
indicate the clock. e.g. monotonic vs unix time.


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

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-http-handler.cc@303
PS1, Line 303:   int64_t duration_us = record.end_time_us - 
record.start_time_us;
that can be negative if settimeofday() occurred in the mean time. What should 
happen in that case?


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

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@622
PS1, Line 622: /// Start and end time of the query
same comment about clock


http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.h@640
PS1, Line 640: Unix milliseconds
like that


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

http://gerrit.cloudera.org:8080/#/c/8305/1/be/src/service/impala-server.cc@1009
PS1, Line 1009:   int64_t duration_ms = duration_us / MICROS_PER_MILLI;
same question (though old code could go negative).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:42:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.

2017-10-17 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8215 )

Change subject: IMPALA-5142 EventSequence displays negative elapsed time.
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8215/1//COMMIT_MSG@22
PS1, Line 22: value.
> In the thread doing ClientRequestState::ClientRequestState()
There is only one call to AddEventSequence("Query Timeline") per query though, 
right?  so how does another thread operate on that event_sequence_ before the 
constructor of CRS() returns?

Are you able to reproduce this problem? If so, can you change your if-stmt to a 
DCHECK() and put the backtrace in the JIRA when the DCHECK fires.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896
Gerrit-Change-Number: 8215
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:30:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

2017-10-17 Thread Joe McDonnell (Code Review) 
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8294/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

http://gerrit.cloudera.org:8080/#/c/8294/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3000
PS1, Line 3000:   AnalysisError(String.format("load data inpath '%s' %s 
into table " +
  :   "tpch.lineitem", 
"s3a://bucket/test-warehouse/test.out", overwrite),
  :   "INPATH location 
's3a://bucket/test-warehouse/test.out' must point to an " +
  :   "HDFS, S3A or ADL filesystem.");
Impala cannot load data from s3n. I think this test is intended to verify our 
error message when given an s3n path, so I don't think an s3a path will work 
here. The source of the error is LoadDataStmt::analyzePaths().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:30:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Add 'psmisc' to bootstrap system.sh.

2017-10-17 Thread Philip Zeyliger (Code Review) 
Philip Zeyliger has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8306


Change subject: Add 'psmisc' to bootstrap_system.sh.
..

Add 'psmisc' to bootstrap_system.sh.

testdata/bin/kill-all.sh uses /usr/bin/killall, which
is provided by the 'psmisc' package on Ubuntu. This
commit adds the package to the list of packages we install.

The Docker base image for Ubuntu 16.04 doesn't contain this package,
which is how this came up.

Change-Id: I378b6fbf7a7b3f0b42ecf064e98cfe6e29b3
---
M bin/bootstrap_system.sh
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I378b6fbf7a7b3f0b42ecf064e98cfe6e29b3
Gerrit-Change-Number: 8306
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

2017-10-17 Thread Michael Brown (Code Review) 
Michael Brown has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/8294/1//COMMIT_MSG@9
PS1, Line 9: JENKINS-1102 added the IAM role ImpalaDev to the Impala Jenkins 
workers
   : to facilitate s3 access without having to carry around AWS 
credentials
   : in environment variables, from where they were prone to escape to 
log
   : files posted in public places.
This looks like something specific to a downstream environment and should be 
removed from the commit message.


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

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@268
PS1, Line 268: test
tests


http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@276
PS1, Line 276: curl -sf --connect-timeout 1 --max-time 5
Are these curl options common across a wide variety of OSs? If they are newer 
and only work under, say, Ubuntu 16, they would be a problem for contributors 
using older distributions.


http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@276
PS1, Line 276: http://169.254.169.254/latest/meta-data/iam/security-credentials/
Is there a AWS reference page you can add as a comment so I can read up more on 
this?


http://gerrit.cloudera.org:8080/#/c/8294/1/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
File testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl:

http://gerrit.cloudera.org:8080/#/c/8294/1/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl@61
PS1, Line 61: 
Maybe have a comment explaining testdata/cluster/admin needs this (and the END) 
as a marker and not to remove?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:09:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

2017-10-17 Thread Lars Volker (Code Review) 
Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8038 )

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 7:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@686
PS7, Line 686: tmp_set
"_set" implies this is a set, but it's actually a dict. I think 
"new_query_options" might be a better name.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@1332
PS7, Line 1332:   There are two types of options: shell options and 
query_options. Both can be set in
nit: missing article


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@28
PS7, Line 28: # EXPLAIN_LEVEL=2
are these case sensitive? Otherwise I'd opt for consistency with the previous 
section.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@36
PS7, Line 36: def convert_loaded_shell_option(option, value, default_options, 
config_filename):
:   """Converts some values based on their type in default_options
:
:  Setting 'config_filename' in the config file would have no 
effect,
:  so its original value is kept.
From looking at the signature and the comment I have no idea what this method 
does. Can you clarify it so it makes sense without further context? The comment 
also should explain what the return value is.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@42
PS7, Line 42: if default_options[option] in [True, False]:
How about "if type(...) is bool:" ?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@58
PS7, Line 58: return config_filename
What if none of the cases match?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@66
PS7, Line 66:   filtered and some values are converted (False, True, None).
Can you explain what exactly gets converted into what? Strings into Python 
values?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@72
PS7, Line 72:   Returns a pair of dictionaries (shell_options, query_options)
Can you add a comment explaining what the keys and values of those dictionaries 
are?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@91
PS7, Line 91: upper
Is this needed? If so, can you add a comment explaining why?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@167
PS7, Line 167:   "The following sections are used: 
[impala], "
I think we should mention that the section titles are case sensitive



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:08:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5599: Clean up references to TimestampValue in be/src/service.

2017-10-17 Thread Zoram Thanga (Code Review) 
Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8305


Change subject: IMPALA-5599: Clean up references to TimestampValue in 
be/src/service.
..

IMPALA-5599: Clean up references to TimestampValue in be/src/service.

This is a follow-on commit to d53f43b4. In this patch we replace all
inappropriate usage of TimestampValue in be/src/service with simpler
data types for Unix timestamps, and where conversions to strings are
needed, we use the interfaces introduced by the previous commit.

Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 26 insertions(+), 39 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I642a1d713597826bb7c15cd2ecb6638cb813a02c
Gerrit-Change-Number: 8305
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga

[Impala-ASF-CR] IMPALA-1575: Yield admission control resources at query end

2017-10-17 Thread Tim Armstrong (Code Review) 
Tim Armstrong has abandoned this change. ( http://gerrit.cloudera.org:8080/7079 
)

Change subject: IMPALA-1575: Yield admission control resources at query end
..


Abandoned

Abandoning this patch since it's no longer up for review. I'll post updated 
patches.
--
To view, visit http://gerrit.cloudera.org:8080/7079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia5003d017b3142a160bacf7e3569ff26026b1700
Gerrit-Change-Number: 7079
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8274 )

Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
..


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24
Gerrit-Change-Number: 8274
Gerrit-PatchSet: 2
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 17 Oct 2017 22:29:35 +
Gerrit-HasComments: No

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

2017-10-17 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7564 )

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


Patch Set 10:

(27 comments)

http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc@455
PS10, Line 455: jbyteArray thrift_in_query_options) {
Not sure what 'thrift_in_query_options' means. How about using 'tquery_options' 
instead? Ideally, we would not need to pass in the existing query options.


http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc@457
PS10, Line 457:   DeserializeThriftMsg(env, thrift_in_query_options, );
Needs error handling, e.g. using THROW_IF_ERROR_RET


http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc@458
PS10, Line 458:   const char *o = env->GetStringUTFChars(options_str, NULL);
* You need to release the string UTF chars, take a look at JniUtfCharGuard in 
jni-util.h

* style: const char* o


http://gerrit.cloudera.org:8080/#/c/7564/10/be/src/service/fe-support.cc@461
PS10, Line 461:   TParseQueryOptionsResult result;
I think it's simpler to convert all Status to an exception. That way we don't 
need the TParseQueryOptionsResult at all, and the error handling is consistent 
(always throws an exception in case of error).


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/Planner.java@119
PS10, Line 119: // Create runtime filters. 'singleNodePlan' now points to 
the root of the distributed
Comment is wrong. 'singleNodePlan' definitely does not point to the root of the 
distributed plan.

Use rootFragment.getPlanRoot() instead


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java:

http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@354
PS10, Line 354: public void addTarget(RuntimeFilterTarget target) { 
targets_.add(target); }
add newline before


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@397
PS10, Line 397: Preconditions.checkState(maxNumFilters >= 0);
Why is 0 not a valid value?


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@423
PS10, Line 423:   filter.computeNdvEstimate();
My understanding is that IMPALA-3450 has been fixed and we can remove this code.


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@537
PS10, Line 537:* The assigned filters are the ones for which 'scanNode' can 
be used a destination
can be used as a destination node


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@539
PS10, Line 539:* 1. If DISABLE_ROW_RUNTIME_FILTERING query option is set, a 
filter is only assigned to
If the DISABLE_ROW_RUNTIME_FILTERING query option is set ...


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@541
PS10, Line 541:* 2. If RUNTIME_FILTER_MODE query option is set to LOCAL, a 
filter is only assigned to
If the RUNTIME_FILTER_MODE query option is set ...


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@542
PS10, Line 542:*'scanNode' if the filter is local to the scan node.
This doesn't really explain what the LOCAL option means. How about:

... a filter is only assigned to 'scanNode' if the filter is produced within 
the same fragment that contains the scan node.


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@563
PS10, Line 563:   if (!isSingleNodeExec && runtimeFilterMode == 
TRuntimeFilterMode.LOCAL &&
Why the !isSingleNodeExec condition? I think that !isLocalTarget implies 
!isSingleNodeExec.


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@576
PS10, Line 576:   static private boolean IsLocalTarget(RuntimeFilter filter, 
ScanNode targetNode) {
isLocalTarget()


http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@583
PS10, Line 583:   static private boolean IsBoundByPartitionColumns(Analyzer 
analyzer, Expr targetExpr,
isBoundByPartitionColumns()

[Impala-ASF-CR] Making bin/bootstrap system.sh executable.

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8292 )

Change subject: Making bin/bootstrap_system.sh executable.
..

Making bin/bootstrap_system.sh executable.

Script should be executable, since it's reasonable to
run it standalone.

Change-Id: I15973f68e0d6cba86da58a104a607cac0627c4cb
Reviewed-on: http://gerrit.cloudera.org:8080/8292
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
M bin/bootstrap_system.sh
1 file changed, 0 insertions(+), 0 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I15973f68e0d6cba86da58a104a607cac0627c4cb
Gerrit-Change-Number: 8292
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple

[Impala-ASF-CR] Making bin/bootstrap system.sh executable.

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8292 )

Change subject: Making bin/bootstrap_system.sh executable.
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15973f68e0d6cba86da58a104a607cac0627c4cb
Gerrit-Change-Number: 8292
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 17 Oct 2017 22:01:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.

2017-10-17 Thread Pranay Singh (Code Review) 
Pranay Singh has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8215 )

Change subject: IMPALA-5142 EventSequence displays negative elapsed time.
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8215/1//COMMIT_MSG@22
PS1, Line 22: value.
> That's right since the code not synchronized we can read a garbage start_ w
In the thread doing ClientRequestState::ClientRequestState()

a) When the function AddEventSequence() is called, an EventSequence with the  
name is looked for, if found then the EventSequence is returned else a new 
event sequence gets created.

b) Then after the query_events->Start(0 resets the monotonic time for the 
EventSequence, so say in this case a matching name for "Query Timeline" was 
found

In another thread which is done doing 
AdmissionController::AdmitQuery(QuerySchedule* schedule)
in the mean time will have ScopedEvent destroyed for the same EventSequence and 
since they are not serialized we may hit upon this issue.

/// Utility class to mark an event when the object is destroyed.
class ScopedEvent {
  . 

  /// Mark the event when the object is destroyed
  ~ScopedEvent() {   
event_sequence_->MarkEvent(label_); --->
  } 


};



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896
Gerrit-Change-Number: 8215
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Tue, 17 Oct 2017 21:55:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4236: Codegen CopyRows() for select nodes

2017-10-17 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8196 )

Change subject: IMPALA-4236: Codegen CopyRows() for select nodes
..


Patch Set 8: Code-Review+1

Will let Michael take another look.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0d496d004418468e16b6f564f90f45ebbf87c1e
Gerrit-Change-Number: 8196
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 17 Oct 2017 21:41:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker 
errors
..


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 17 Oct 2017 21:03:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

2017-10-17 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker 
errors
..


Patch Set 6: Code-Review+2

Carrying over Tim's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:49:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

2017-10-17 Thread Bikramjeet Vig (Code Review) 
Hello Philip Zeyliger, anujphadke, Tim Armstrong,

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

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

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

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker 
errors
..

IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.

Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.

Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/testutil/gtest-util.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
6 files changed, 93 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors

2017-10-17 Thread Bikramjeet Vig (Code Review) 
Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker 
errors
..


Patch Set 4:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@308
PS4, Line 308: if (!diagnostic_err.empty()) ss <<" "<< diagnostic_err;
> nit: Do we do spacing around "<<" operators?
Done


http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@1625
PS4, Line 1625: diagnostic_printer <<"LLVM diagnostic error: ";
> nit: space around "<<"?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:29:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

2017-10-17 Thread Laszlo Gaal (Code Review) 
Laszlo Gaal has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8294 )

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@240
PS1, Line 240: #export AWS_SECRET_ACCESS_KEY="${AWS_SECRET_ACCESS_KEY-}"
 : #export AWS_ACCESS_KEY_ID="${AWS_ACCESS_KEY_ID-}"
Self-inflicted review finding: remove commented-out lines.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 17 Oct 2017 19:45:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

2017-10-17 Thread Laszlo Gaal (Code Review) 
Laszlo Gaal has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8294


Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..

IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

JENKINS-1102 added the IAM role ImpalaDev to the Impala Jenkins workers
to facilitate s3 access without having to carry around AWS credentials
in environment variables, from where they were prone to escape to log
files posted in public places.

This change paves the way for Impala build and test jobs to use the IAM
roles to access s3 buckets. There are a few minor changes that allow
this to happen:

Changes to the configuration script:
1. bin/impala-config.sh stops setting the AWS_* environment variables
   to dummy default values. When AWS credentials are not supplied in
   the environment variables AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY,
   these variables are unset (removed from the environment), otherwise
   they would preempt authentication based on the IAM role.
2. Having AWS credentials in the AWS_* environment variables is now
   optional. They are still accepted to allow for private test runs
   accessing private/nondefault buckets with custom credentials.
3. bin/impala-config.sh now checks if credentials are supplied in the
   AWS_* variables or via the IAM role.

Changes to the frontend tests:
1. Some front-end tests still referenced the old s3 connector s3n:,
   this connector does not support s3 auth via IAM roles. These
   locations are changed to use the newer s3a:, which is the connector
   capable of using IAM roles for authentication and which is used
   in all other code locations.

Changes to the minicluster setup:
1. As a corollary the s3n: configuration sections are removed from
   core-site.xml.tmpl.
2. Remove empty AWS credentials from core-site.xml.tmpl:

   The minicluster setup script susbstitutes values from environment
   variables into Hadoop *-site.xml config files when setting up
   the minicluster runtime environment. The configuration file
   core-site.xml.tmpl contains a section for s3 access, including
   AWS credentials.

   Impala can now use IAM roles for s3 access; this requires the removal
   of environment variables holding AWS credentials, which
   1. breaks the substitution logic in testdata/cluster/admin, and
   2. would break the IAM-based credentials if empty credentials were
  supplied in core-site.xml

   The fix for all of the above issues is to remove the AWS credential
   settings from the generated core-site.xml if both AWS_ACCESS_KEY_ID and
   AWS_SECRET_ACCESS_KEY environment variables are absent or empty.

Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
---
M bin/impala-config.sh
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M testdata/cluster/admin
M testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
5 files changed, 52 insertions(+), 29 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal

[Impala-ASF-CR] Making bin/bootstrap system.sh executable.

2017-10-17 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8292 )

Change subject: Making bin/bootstrap_system.sh executable.
..


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15973f68e0d6cba86da58a104a607cac0627c4cb
Gerrit-Change-Number: 8292
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Tue, 17 Oct 2017 18:03:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5142 EventSequence displays negative elapsed time.

2017-10-17 Thread Dan Hecht (Code Review) 
Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8215 )

Change subject: IMPALA-5142 EventSequence displays negative elapsed time.
..


Patch Set 1:

> (1 comment)

But who is the caller of MarkEvent()? How can that be happening in concurrently 
with the construction of the ClientRequestState()? No one would have a 
reference to it yet...


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8c944396d96473b17b453da3e913ffc56680a896
Gerrit-Change-Number: 8215
Gerrit-PatchSet: 1
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Tue, 17 Oct 2017 16:05:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5058: Improve concurrency of DDL/DML during catalog updates

2017-10-17 Thread Dimitris Tsirogiannis (Code Review) 
Dimitris Tsirogiannis has abandoned this change. ( 
http://gerrit.cloudera.org:8080/8266 )

Change subject: IMPALA-5058: Improve concurrency of DDL/DML during catalog 
updates
..


Abandoned

There are a number of serious issues in this patch. Abandoning for now.
--
To view, visit http://gerrit.cloudera.org:8080/8266
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I16ae27be79eb0c5a9648a15d368ca60a7d04507b
Gerrit-Change-Number: 8266
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis