[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
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
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
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.
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
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
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+
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
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
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
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
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
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.
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.
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
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.
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
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
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.
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
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+
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
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, &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() http://gerrit.cloudera.org:8080/#/c/7564/10/fe/src/main/java/org/apach
[Impala-ASF-CR] Making bin/bootstrap system.sh executable.
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.
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.
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
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
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
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
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
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
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
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.
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.
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
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