[Impala-ASF-CR] IMPALA-8484: Run queries on disjoint executor groups
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13550 ) Change subject: IMPALA-8484: Run queries on disjoint executor groups .. Patch Set 9: (16 comments) Reviewed the remaining tests except the details of tests/util http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py File tests/custom_cluster/test_auto_scaling.py: http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@76 PS9, Line 76: Is it worth validating that enough executors actually got started and the query concurrency was higher than a single executor group could support? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@99 PS9, Line 99: 10 make this a constant? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@101 PS9, Line 101: " \ Could use """ multiline strings instead of line continuation http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@101 PS9, Line 101: query = "select * from functional_parquet.alltypestiny where month < 3 " \ Maybe briefly explain why the query was chosen and how long it should run? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@125 PS9, Line 125: # Must reach 3 but not exceed it Might be worth also sampling the number of running executors to verify directly that none were started up. This is more of a sanity check on AutoScaler than a test for Impala itself I guess. http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@126 PS9, Line 126: 3 This is the same as executor_slots, right? I'd probably use a named constant to make the relationship obvious http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@148 PS9, Line 148: 3 Make this a constant too? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@152 PS9, Line 152: 10 Make this a constant? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@154 PS9, Line 154: query = "select * from functional_parquet.alltypestiny where month < 3 " \ Factor query into constant? Looks like it's reused in multiple tests. http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_auto_scaling.py@156 PS9, Line 156: 5 It's expected that two executor groups should start up, right? Can we assert on that? http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: PS9: Have you looped these tests at all? Would be good to flush out any flakiness (I've been keeping an eye out while reviewing and didn't see anything obvious). http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py@31 PS9, Line 31: down by adding and removing groups of executors. All tests start with a base cluster Nice! http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py@47 PS9, Line 47: # Start a fresh cluster for subsequent tests. This shouldn't be necessary and adds to latency - subsequent tests in the normal test run will start their own cluster. It looks like some other tests do this already but it is an anti-pattern as far as I can see. Looks like this was also added in a couple of other custom cluster tests. http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py@102 PS9, Line 102: qeuued queued http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py@219 PS9, Line 219: test_concurrency_limit maybe test_executor_concurrency() so the relationship with the above coordinator test is more obvious. http://gerrit.cloudera.org:8080/#/c/13550/9/tests/custom_cluster/test_executor_groups.py@256 PS9, Line 256: def test_sequential_startup(self): I'm not convinced that this offers much coverage in addition to tests that start the executors in one batch (which tests multiple executors racing to come online) and the below test (which should test sequential startup with more gaps between queries). Ok to ignore though. -- To view, visit http://gerrit.cloudera.org:8080/13550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a1d0900f2a82bd2fc0a906cc094e442cffa189b Gerrit-Change-Number: 13550 Gerrit-PatchSet: 9 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 07:22:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
Hello Attila Jeges, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13722
to look at the new patch set (#10).
Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
..
IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
This enhancement introduces FORMAT clause for CAST() operator that is
applicable for casts between string types and timestamp types. Instead
of accepting SimpleDateFormat patterns the FORMAT clause supports
datetime patterns following the ISO:SQL:2016 standard.
Note, the CAST() operator without the FORMAT clause still uses
Impala's implementation of SimpleDateFormat handling. Similarly, the
existing conversion functions such as to_timestamp(), from_timestamp()
etc. remain unchanged and use SimpleDateFormat. Contrary to how these
functions work the FORMAT clause is a string literal provided by the
user and its value can't come from a column.
Milestone 1 contains all the format tokens covered by the SQL
standard. Further milestones will add more functionality on top of
this list to cover functionality provided by other RDBMS systems.
List of tokens implemented by this change:
- , YYY, YY, Y: Year tokens
- , RR: Round year tokens
- MM: Month
- DD: Day
- DDD: Day of year
- HH, HH12: Hour of day (1-12)
- HH24: Hour of day (0-23)
- MI: Minute
- SS: Second
- S: Second of day
- FF, FF1, ..., FF9: Fractional second
- AM, PM, A.M., P.M.: Meridiem indicators
- TZH: Timezone hour
- TZM: Timezone minute
- Separators: - . / , ' ; : space
- ISO8601 date indicators (T, Z)
Some notes about the matching algorithm:
- The parsing algorithm uses these tokens in a case insensitive
manner.
- The separators are interchangeable with each other. For example a
'-' separator in the format will match with a '.' character in the
input.
- The length of the separator sequences is handled flexibly meaning
that a single separator character in the format for instance would
match with a multi-separator sequence in the input.
- In a string type to timestamp conversion the timezone offset tokens
are parsed, expected to match with the input but they don't adjust
the result as the input is already expected to be in UTC format.
Usage example:
SELECT CAST('01-02-2019' AS TIMESTAMP FORMAT 'MM-DD-');
SELECT CAST('2019.10.10 13:30:40.123456 +01:30' AS TIMESTAMP
FORMAT '-MM-DD HH24:MI:SS.FF9 TZH:TZM');
SELECT CAST(timestamp_column as STRING
FORMAT " MM HH12 YY") from some_table;
Change-Id: I19d8d097a45ae6f103b6cd1b2d81aad38dfd9e23
---
M be/src/benchmarks/convert-timestamp-benchmark.cc
M be/src/benchmarks/parse-timestamp-benchmark.cc
M be/src/common/init.cc
M be/src/exec/text-converter.inline.h
M be/src/exprs/CMakeLists.txt
A be/src/exprs/cast-expr.cc
A be/src/exprs/cast-expr.h
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/date-functions-ir.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/scalar-expr-evaluator.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/date-parse-util.cc
M be/src/runtime/date-parse-util.h
M be/src/runtime/date-test.cc
M be/src/runtime/date-value.cc
M be/src/runtime/date-value.h
A be/src/runtime/datetime-iso-sql-format-parser.cc
A be/src/runtime/datetime-iso-sql-format-parser.h
A be/src/runtime/datetime-iso-sql-format-tokenizer.cc
A be/src/runtime/datetime-iso-sql-format-tokenizer.h
D be/src/runtime/datetime-parse-util.h
A be/src/runtime/datetime-parser-common.cc
A be/src/runtime/datetime-parser-common.h
R be/src/runtime/datetime-simple-date-format-parser.cc
A be/src/runtime/datetime-simple-date-format-parser.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
M be/src/runtime/timestamp-test.cc
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/impala-server.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M be/src/testutil/random-vector-generators.h
M be/src/util/dict-test.cc
M be/src/util/min-max-filter-test.cc
M be/src/util/string-parser.h
M common/thrift/Exprs.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
A
testdata/workloads/functional-query/queries/QueryTest/cast_format_from_table.test
M testdata/workloads/functional-query/queries/QueryTest/date.test
A tests/query_test/test_cast_with_format.py
54 files changed, 3,434 insertions(+), 861 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/13722/10
--
To view, visit
[Impala-ASF-CR] IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
Gabor Kaszab has posted comments on this change. (
http://gerrit.cloudera.org:8080/13722 )
Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
..
Patch Set 10:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-expr.h
File be/src/exprs/cast-expr.h:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-expr.h@28
PS9, Line 28: class CastFormatExpr : public ScalarFnCall {
> nit: Move this comment in front of L31.
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-functions-ir.cc@171
PS9, Line 171: DCHECK(ctx != nullptr);
> Add DCHECK(ctx != nullptr);
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-functions-ir.cc@178
PS9, Line 178:
> It would be better to use ToString() directly rather than involving the ind
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-functions-ir.cc@191
PS9, Line 191: StringVal CastFunctions::CastToStringVal(FunctionContext* ctx,
const DateVal& val) {
> Add DCHECK(ctx != nullptr);
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/exprs/cast-functions-ir.cc@347
PS9, Line 347:
> Add DCHECK(ctx != nullptr);
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.h
File be/src/runtime/datetime-iso-sql-format-parser.h:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.h@28
PS9, Line 28: //
> nit: in most other header files comments are prefixed with ///
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.h@46
PS9, Line 46: posi
> position
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.h@47
PS9, Line 47: ext token t
> It hasn't been properly defined what "token group" means. Whst's the differ
Nothing, I use them as synonyms :)
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.h@48
PS9, Line 48: GetNextTokenFromInput(cons
> Naming is a bit confusing as it is used to find the end of the current toke
Done
http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@116
PS3, Line 116: break;
> Maybe add some DCHECKs at the beginning of the function then?
Good idea. Added a DCHECK for result->hour.
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.cc
File be/src/runtime/datetime-iso-sql-format-parser.cc:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-parser.cc@47
PS9, Line 47:if (tok->type == SEPARATOR) {
: bool res = ProcessSeparators(¤t_pos, end_pos, dt_ctx,
&i, &tok);
: if (!res || current_pos == end_pos) return res;
: }
:
: const char* token_end_pos = GetNextTokenFromInput(current_pos,
: end_pos - current_pos, *tok);
: if (token_end_pos == nullptr) return false;
: int token_len = token_end_pos - current_pos;
:
: switch(tok->type) {
: case YEAR: {
: if (!ParseAndValidate(current_pos, token_len, 0, ,
&result->year)) {
: return false;
: }
: if (token_len < 4) {
: PrefixYearFromCurrentYear(token_len,
dt_ctx.current_time, result);
: }
: break;
: }
: case ROUND_YEAR: {
:
> Please consider moving this block to a separate function.
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.h
File be/src/runtime/datetime-iso-sql-format-tokenizer.h:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.h@73
PS9, Line 73: deci
> nit: decide
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.h@75
PS9, Line 75: token matc
> Again, the wording here and below is a bit confusing. Shouldn't it just be
I'll use the naming 'token' instead.
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.h@91
PS9, Line 91: found i
> nit: found
Done
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.cc
File be/src/runtime/datetime-iso-sql-format-tokenizer.cc:
http://gerrit.cloudera.org:8080/#/c/13722/9/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@110
PS9, Line 110: unsigned
> I think it would be safer to just use 'int' here.
See below.
http
[Impala-ASF-CR] IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13722 ) Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1 .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3893/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19d8d097a45ae6f103b6cd1b2d81aad38dfd9e23 Gerrit-Change-Number: 13722 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 08:54:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8516: Fix the sha512sum check for the Maven download
Laszlo Gaal has uploaded this change for review. (
http://gerrit.cloudera.org:8080/13876
Change subject: IMPALA-8516: Fix the sha512sum check for the Maven download
..
IMPALA-8516: Fix the sha512sum check for the Maven download
This is a follow-up fix for commit 327b938214:
When bin/bootstrap_system.sh downloads Maven, it performs a checksum
check on the downloaded archive before expanding it. The check is
performed using sha512sum, which has quite particular requirements
for its inputs:
When checking, the input should be a former output of this program.
The default mode is to print a line with checksum, a character
indicating input mode ('*' for binary, space for text),
and name for each FILE.
In our case the checksum lines ended up having two space characters
between the checksum and athe file name. Unfortunately, when the
original commit copied the maven download-and-install stanza to
the new location, the second whitespace was dropped, which made this
line fail on CentOS 6, where the sha512sum implementation takes the
above quoted requirement literally.
This patch restores the extra whitespace, fixing Maven installation
for CentOS 6.
Change-Id: Ic8aa4d78b34d60a01fc8f6afc336d66ee5a005cb
---
M bin/bootstrap_system.sh
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/13876/1
--
To view, visit http://gerrit.cloudera.org:8080/13876
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8aa4d78b34d60a01fc8f6afc336d66ee5a005cb
Gerrit-Change-Number: 13876
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal
[Impala-ASF-CR] IMPALA-8516: Fix the sha512sum check for the Maven download
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13876 ) Change subject: IMPALA-8516: Fix the sha512sum check for the Maven download .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13876/1/bin/bootstrap_system.sh File bin/bootstrap_system.sh: http://gerrit.cloudera.org:8080/#/c/13876/1/bin/bootstrap_system.sh@250 PS1, Line 250: sha512sum -c - <<< '2a803f578f341e164f6753e410413d16ab60fabe31dc491d1fe35c984a5cce696bc71f57757d4538fe7738be04065a216f3ebad4ef7e0ce1bb4c51bc36d6be86 apache-maven-3.5.4-bin.tar.gz' line too long (182 > 90) -- To view, visit http://gerrit.cloudera.org:8080/13876 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8aa4d78b34d60a01fc8f6afc336d66ee5a005cb Gerrit-Change-Number: 13876 Gerrit-PatchSet: 1 Gerrit-Owner: Laszlo Gaal Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 12:07:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG@9 PS4, Line 9: Fixed the buffer overflow that the previous attempt (Change-Id: > Can you add a link to the previous attempt? I linked the previous attempt with its change id. Should I also include the commit hash? I added the information you requested. -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 12:16:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Daniel Becker has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. IMPALA-8710: Increase allowed bit width to 64 for bit packing Fixed the buffer overflow that the previous attempt (Change-Id: I3ff3b387cbe8e41dd78c45411ef54de4afa8) introduced. Compared to that change, only bit-packing.inline.h is different. The tests went into the buffer overflow path but it only produced an error in the ASAN builds. Increasing the allowed bit width for bit packing and bit unpacking to 64 bits. This will be needed to support the Parquet delta encoding. Added new methods to BitWriter and BatchedBitReader handling Uleb and ZigZag integers for 64 bits, also needed by delta encoding. The performance of bit unpacking is either the same or better with the new implementation, depending on bit width. Updated the results in bit-packing-benchmark.cc. Testing: - Modified bit packing and unpacking tests to test bit widths up to 64 bits. - Tests covering the additions in BitWriter and BatchedBitReader. Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b --- M be/src/benchmarks/bit-packing-benchmark.cc M be/src/util/CMakeLists.txt M be/src/util/bit-packing-test.cc M be/src/util/bit-packing.h M be/src/util/bit-packing.inline.h M be/src/util/bit-stream-utils-test.cc M be/src/util/bit-stream-utils.h M be/src/util/bit-stream-utils.inline.h M be/src/util/rle-encoding.h M be/src/util/rle-test.cc 10 files changed, 430 insertions(+), 209 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/13809/5 -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-8516: Fix the sha512sum check for the Maven download
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13876 ) Change subject: IMPALA-8516: Fix the sha512sum check for the Maven download .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3894/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13876 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8aa4d78b34d60a01fc8f6afc336d66ee5a005cb Gerrit-Change-Number: 13876 Gerrit-PatchSet: 1 Gerrit-Owner: Laszlo Gaal Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 12:46:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 5: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG@9 PS4, Line 9: Fixed the buffer overflow that the previous attempt (Change-Id: > I linked the previous attempt with its change id. Should I also include the Instead of the change-id I think it's better to link the commit hash or maybe the URL to the code review. -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 12:49:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3895/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 12:57:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13809/5/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: http://gerrit.cloudera.org:8080/#/c/13809/5/be/src/util/bit-packing.inline.h@196 PS5, Line 196: constexpr bool READ_32_BITS = WORDS_TO_READ == 1 Can we safely read 32 bits when it is not a full batch? -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 12:59:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Daniel Becker has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. IMPALA-8710: Increase allowed bit width to 64 for bit packing Fixed the buffer overflow that the previous attempt (commit b1cbf9e6b786132e86699cbb1e472ec98499bb11, https://gerrit.cloudera.org/#/c/13737) introduced. Compared to that change, only bit-packing.inline.h is different. The tests went into the buffer overflow path but it only produced an error in the ASAN builds. Increasing the allowed bit width for bit packing and bit unpacking to 64 bits. This will be needed to support the Parquet delta encoding. Added new methods to BitWriter and BatchedBitReader handling Uleb and ZigZag integers for 64 bits, also needed by delta encoding. The performance of bit unpacking is either the same or better with the new implementation, depending on bit width. Updated the results in bit-packing-benchmark.cc. Testing: - Modified bit packing and unpacking tests to test bit widths up to 64 bits. - Tests covering the additions in BitWriter and BatchedBitReader. Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b --- M be/src/benchmarks/bit-packing-benchmark.cc M be/src/util/CMakeLists.txt M be/src/util/bit-packing-test.cc M be/src/util/bit-packing.h M be/src/util/bit-packing.inline.h M be/src/util/bit-stream-utils-test.cc M be/src/util/bit-stream-utils.h M be/src/util/bit-stream-utils.inline.h M be/src/util/rle-encoding.h M be/src/util/rle-test.cc 10 files changed, 430 insertions(+), 209 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/13809/6 -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13809/4//COMMIT_MSG@9 PS4, Line 9: Fixed the buffer overflow that the previous attempt (commit > Instead of the change-id I think it's better to link the commit hash or may I added both. http://gerrit.cloudera.org:8080/#/c/13809/5/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: http://gerrit.cloudera.org:8080/#/c/13809/5/be/src/util/bit-packing.inline.h@196 PS5, Line 196: constexpr bool READ_32_BITS = WORDS_TO_READ == 1 > Can we safely read 32 bits when it is not a full batch? We can because BitPacking::UnpackUpTo31Values, which calls this function, DCHECKs the length of the input buffer. -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 13:18:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 13:23:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8606: Don't load table meta for GET TABLES in local catalog mode
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13874 Change subject: IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode .. IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode In LocalCatalog implementation, LocalDb#getTable will always return a completely loaded table containing all the meta of columns, partitions, files, etc. It's time consuming if we implement the GET_TABLES HiveServer2 operation based on this interface, since GET_TABLES only requires table names, table types and table comments, while this interface will trigger catalogd to fully load the table meta. It becomes worse when we do this for all the tables. This patch introduces a new interface, getTableIfCached, to return a LocalIncompleteTable object if the corresponding table is unloaded, which requires no round trips to the catalogd. It's used to boost the GET_TABLES performance in LocalCatalog mode. Tests - Testing in a HMS with 100 dbs and 3000 tables, without this patch it takes ~2mins in GET_TABLES on a cold started cluster. With this patch, the time reduces to ~1s. Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb --- M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java A fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M tests/hs2/test_hs2.py 10 files changed, 106 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/13874/4 -- To view, visit http://gerrit.cloudera.org:8080/13874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb Gerrit-Change-Number: 13874 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang
[Impala-ASF-CR] IMPALA-8710: Increase allowed bit width to 64 for bit packing
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13809 ) Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3896/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9276ca291e3d36db16f63f8caf3e9248a18d85b Gerrit-Change-Number: 13809 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 13:58:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8606: Don't load table meta for GET TABLES in local catalog mode
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13874 ) Change subject: IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/3897/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/13874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb Gerrit-Change-Number: 13874 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 14:12:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Tamas Mate has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. IMPALA-8750: Fix profile observability tests IMPALA-8443 adds a test 'test_query_profile_contains_query_compilation_events' which can fail because there are two parts of the 'Query Compilation' events that can change based on the build environment. 1) Whether the metadata is cached or not before the test starts. 2) Whether 'lineage_event_log_dir' is configured on the cluster. This change covers these scenarios by splitting the tests into sepearate ones where the catalog cache is pre-evicted/pre-loaded and taking into consideration the current cluster configuration. Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a --- M tests/common/impala_service.py M tests/query_test/test_observability.py 2 files changed, 56 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/13851/3 -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate
[Impala-ASF-CR] IMPALA-8606: Don't load table meta for GET TABLES in local catalog mode
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13874 to look at the new patch set (#5). Change subject: IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode .. IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode In LocalCatalog implementation, LocalDb#getTable will always return a completely loaded table containing all the meta of columns, partitions, files, etc. It's time consuming if we implement the GET_TABLES HiveServer2 operation based on this interface, since GET_TABLES only requires table names, table types and table comments, while this interface will trigger catalogd to fully load the table meta. It becomes worse when we do this for all the tables. This patch introduces a new interface, getTableIfCached, to return a LocalIncompleteTable object if the corresponding table is unloaded, which requires no round trips to the catalogd. It's used to boost the GET_TABLES performance in LocalCatalog mode. Tests - Testing in a HMS with 100 dbs and 3000 tables, without this patch it takes ~2mins in GET_TABLES on a cold started cluster. With this patch, the time reduces to ~1s. Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb --- M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/catalog/FeCatalog.java M fe/src/main/java/org/apache/impala/catalog/FeDb.java M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java A fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java M fe/src/main/java/org/apache/impala/service/MetadataOp.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M tests/hs2/test_hs2.py 10 files changed, 122 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/13874/5 -- To view, visit http://gerrit.cloudera.org:8080/13874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb Gerrit-Change-Number: 13874 Gerrit-PatchSet: 5 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8606: Don't load table meta for GET TABLES in local catalog mode
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13874 ) Change subject: IMPALA-8606: Don't load table meta for GET_TABLES in local catalog mode .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3899/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia8bbab7efdf8e629abe09d89ae3bd770e3feaccb Gerrit-Change-Number: 13874 Gerrit-PatchSet: 5 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 14:58:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3898/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Wed, 17 Jul 2019 14:59:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13873 to look at the new patch set (#3). Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations Refactors PlanRootSink into a base class with two subclasses: BlockingPlanRootSink and BufferedPlanRootSink. BlockingPlanRootSink encapsulates the current implementation of PlanRootSink and BufferedPlanRootSink encapsulates a new implementation that will buffer RowBatches in memory until they are read by the client. The implementation of BlockingPlanRootSink is left to future work. A new query option called POOL_QUERY_RESULTS controls whether a BlockingPlanRootSink or a BufferedPlanRootSink is used as the DataSink. POOL_QUERY_RESULTS is false by default. Added a few more docs to PlanRootSink and BlockingPlanRootSink to make the implementation easier to understand. Testing: * Since no new functionality has been added no tests were added * Ran core tests Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca --- M be/src/exec/CMakeLists.txt A be/src/exec/blocking-plan-root-sink.cc A be/src/exec/blocking-plan-root-sink.h A be/src/exec/buffered-plan-root-sink.cc A be/src/exec/buffered-plan-root-sink.h M be/src/exec/data-sink.cc M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift 12 files changed, 369 insertions(+), 154 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/13873/3 -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Hello Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13851 to look at the new patch set (#4). Change subject: IMPALA-8750: Fix profile observability tests .. IMPALA-8750: Fix profile observability tests IMPALA-8443 adds a test 'test_query_profile_contains_query_compilation_events' which can fail because there are two parts of the 'Query Compilation' events that can change based on the build environment. 1) Whether the metadata is cached or not before the test starts. 2) Whether 'lineage_event_log_dir' is configured on the cluster. This change covers these scenarios by splitting the tests into sepearate ones where the catalog cache is pre-evicted/pre-loaded and taking into consideration the current cluster configuration. Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a --- M tests/common/impala_service.py M tests/query_test/test_observability.py 2 files changed, 57 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/13851/4 -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13851/4/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/13851/4/tests/common/impala_service.py@88 PS4, Line 88: f flake8: E501 line too long (96 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Wed, 17 Jul 2019 15:34:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Hello Fredy Wijaya, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13851 to look at the new patch set (#5). Change subject: IMPALA-8750: Fix profile observability tests .. IMPALA-8750: Fix profile observability tests IMPALA-8443 adds a test 'test_query_profile_contains_query_compilation_events' which can fail because there are two parts of the 'Query Compilation' events that can change based on the build environment. 1) Whether the metadata is cached or not before the test starts. 2) Whether 'lineage_event_log_dir' is configured on the cluster. This change covers these scenarios by splitting the tests into sepearate ones where the catalog cache is pre-evicted/pre-loaded and taking into consideration the current cluster configuration. Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a --- M tests/common/impala_service.py M tests/query_test/test_observability.py 2 files changed, 57 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/13851/5 -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3900/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Wed, 17 Jul 2019 16:13:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3901/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Wed, 17 Jul 2019 16:47:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 3: (6 comments) Mostly looks good, just some comments about the feature flag and some code cleanup we should do to match newer best practices in the codebase http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG@24 PS3, Line 24: * Since no new functionality has been added no tests were added Can you add a test that exercises the option to confirm that it doesn't crash when you run a query? We don't want user-toggleable options that can cause instability. I actually like this approach of doing the development behind a feature flag; just gotta make sure that we keep the code in a state where we could cut a release from it if needed. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h File be/src/exec/blocking-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@18 PS3, Line 18: #ifndef IMPALA_EXEC_BLOCKING_PLAN_ROOT_SINK_H #pragma once, since that's what we're doing in new code http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@56 PS3, Line 56: virtual Status Send(RuntimeState* state, RowBatch* batch); Can you add override annotations - that's a good practice for new code. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@19 PS3, Line 19: #define IMPALA_EXEC_BUFFERED_PLAN_ROOT_SINK_H #pragma once http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@34 PS3, Line 34: virtual Status Send(RuntimeState* state, RowBatch* batch); Can you add override annotations - that's a good practice for new code. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h@175 PS3, Line 175: ADVANCED This should be a DEVELOPMENT query option at least until it's ready. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 17:39:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8489: Partitions created by RECOVER PARTITIONS fail to create insert events with IllegalStateException.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13860 ) Change subject: IMPALA-8489: Partitions created by RECOVER PARTITIONS fail to create insert events with IllegalStateException. .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/13860/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13860/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3844 PS1, Line 3844: loadAllPartitions Is there a reason to load all the partitions? Can we fetch the partitions by name using HDFSTable#getPartitionsForNames(partitionFilesMapBeforeInsert.keySet()) http://gerrit.cloudera.org:8080/#/c/13860/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3849 PS1, Line 3849: ALTER Do you mean add partition events? http://gerrit.cloudera.org:8080/#/c/13860/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3851 PS1, Line 3851: newPartitionNames IIUC, this is a list of all the partitionNames which are not affected by this insert operation. Not necessarily, new partitions. Is that right? http://gerrit.cloudera.org:8080/#/c/13860/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3855 PS1, Line 3855: partitionNamesMapPostInsert Its not clear why this is being done. Can you clarify? -- To view, visit http://gerrit.cloudera.org:8080/13860 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idef7f6aadff2868047c861ebfcc05d65f080eab9 Gerrit-Change-Number: 13860 Gerrit-PatchSet: 1 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 17 Jul 2019 18:05:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8751: Skip failing Kudu - HMS integration tests with Hive 3
Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13854 ) Change subject: IMPALA-8751: Skip failing Kudu - HMS integration tests with Hive 3 .. Patch Set 1: Code-Review+1 To skip these kudu tests is fine to me. -- To view, visit http://gerrit.cloudera.org:8080/13854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic19b8d93eaa6e8ec886c5704578563fb0871f941 Gerrit-Change-Number: 13854 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Yongzhi Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 17 Jul 2019 18:14:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13873 to look at the new patch set (#4). Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations Refactors PlanRootSink into a base class with two subclasses: BlockingPlanRootSink and BufferedPlanRootSink. BlockingPlanRootSink encapsulates the current implementation of PlanRootSink and BufferedPlanRootSink encapsulates a new implementation that will buffer RowBatches in memory until they are read by the client. The implementation of BlockingPlanRootSink is left to future work. A new query option called POOL_QUERY_RESULTS controls whether a BlockingPlanRootSink or a BufferedPlanRootSink is used as the DataSink. POOL_QUERY_RESULTS is false by default. Added a few more docs to PlanRootSink and BlockingPlanRootSink to make the implementation easier to understand. Testing: * Since no new functionality has been added no tests were added * Running core tests Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca --- M be/src/exec/CMakeLists.txt A be/src/exec/blocking-plan-root-sink.cc A be/src/exec/blocking-plan-root-sink.h A be/src/exec/buffered-plan-root-sink.cc A be/src/exec/buffered-plan-root-sink.h M be/src/exec/data-sink.cc M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift A tests/query_test/test_result_spooling.py 13 files changed, 397 insertions(+), 154 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/13873/4 -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13873/4/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/13873/4/tests/query_test/test_result_spooling.py@18 PS4, Line 18: import pytest flake8: F401 'pytest' imported but unused http://gerrit.cloudera.org:8080/#/c/13873/4/tests/query_test/test_result_spooling.py@22 PS4, Line 22: class TestResultSpooling(ImpalaTestSuite): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/13873/4/tests/query_test/test_result_spooling.py@31 PS4, Line 31: flake8: E203 whitespace before ':' -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 19:01:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13873 to look at the new patch set (#5). Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations Refactors PlanRootSink into a base class with two subclasses: BlockingPlanRootSink and BufferedPlanRootSink. BlockingPlanRootSink encapsulates the current implementation of PlanRootSink and BufferedPlanRootSink encapsulates a new implementation that will buffer RowBatches in memory until they are read by the client. The implementation of BlockingPlanRootSink is left to future work. A new query option called POOL_QUERY_RESULTS controls whether a BlockingPlanRootSink or a BufferedPlanRootSink is used as the DataSink. POOL_QUERY_RESULTS is false by default. Added a few more docs to PlanRootSink and BlockingPlanRootSink to make the implementation easier to understand. Testing: * Added tests/query_test/test_result_spooling.py; currently only runs a simple select limit 10 with SPOOL_QUERY_RESULTS = true and validates that 0 rows are returned * Ran core tests Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca --- M be/src/exec/CMakeLists.txt A be/src/exec/blocking-plan-root-sink.cc A be/src/exec/blocking-plan-root-sink.h A be/src/exec/buffered-plan-root-sink.cc A be/src/exec/buffered-plan-root-sink.h M be/src/exec/data-sink.cc M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift A tests/query_test/test_result_spooling.py 13 files changed, 397 insertions(+), 154 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/13873/5 -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG@24 PS3, Line 24: * Since no new functionality has been added no tests were added > Can you add a test that exercises the option to confirm that it doesn't cra Done - added tests/query_test/test_result_spooling.py - doesn't do much yet, just validates that running a simple query with SPOOL_QUERY_RESULTS = true does not crash Impala. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h File be/src/exec/blocking-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@18 PS3, Line 18: #ifndef IMPALA_EXEC_BLOCKING_PLAN_ROOT_SINK_H > #pragma once, since that's what we're doing in new code Done http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@56 PS3, Line 56: virtual Status Send(RuntimeState* state, RowBatch* batch); > Can you add override annotations - that's a good practice for new code. Done http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@19 PS3, Line 19: #define IMPALA_EXEC_BUFFERED_PLAN_ROOT_SINK_H > #pragma once Done http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@34 PS3, Line 34: virtual Status Send(RuntimeState* state, RowBatch* batch); > Can you add override annotations - that's a good practice for new code. Done http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h@175 PS3, Line 175: ADVANCED > This should be a DEVELOPMENT query option at least until it's ready. Done -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 19:04:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13873 to look at the new patch set (#6). Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations Refactors PlanRootSink into a base class with two subclasses: BlockingPlanRootSink and BufferedPlanRootSink. BlockingPlanRootSink encapsulates the current implementation of PlanRootSink and BufferedPlanRootSink encapsulates a new implementation that will buffer RowBatches in memory until they are read by the client. The implementation of BlockingPlanRootSink is left to future work. A new query option called POOL_QUERY_RESULTS controls whether a BlockingPlanRootSink or a BufferedPlanRootSink is used as the DataSink. POOL_QUERY_RESULTS is false by default. Added a few more docs to PlanRootSink and BlockingPlanRootSink to make the implementation easier to understand. Testing: * Added tests/query_test/test_result_spooling.py; currently only runs a simple select limit 10 with SPOOL_QUERY_RESULTS = true and validates that 0 rows are returned * Ran core tests Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca --- M be/src/exec/CMakeLists.txt A be/src/exec/blocking-plan-root-sink.cc A be/src/exec/blocking-plan-root-sink.h A be/src/exec/buffered-plan-root-sink.cc A be/src/exec/buffered-plan-root-sink.h M be/src/exec/data-sink.cc M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/service/query-options.cc M be/src/service/query-options.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift A tests/query_test/test_result_spooling.py 13 files changed, 396 insertions(+), 154 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/13873/6 -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13872 ) Change subject: IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4608/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/13872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I08f1c36ecf54ac277d99e2d2843163eada732e50 Gerrit-Change-Number: 13872 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 17 Jul 2019 19:09:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/13872 ) Change subject: IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage .. Patch Set 1: Investigating whether this works on upstream infrastructure. -- To view, visit http://gerrit.cloudera.org:8080/13872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I08f1c36ecf54ac277d99e2d2843163eada732e50 Gerrit-Change-Number: 13872 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Wed, 17 Jul 2019 19:11:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3902/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 19:40:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3903/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 19:47:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 6: Code-Review+1 LGTM. I can upgrade to +2 but will wait to see if Michael also wants to take a look. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 20:22:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8741: Speed up bit unpacking by vectorisation
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13807 ) Change subject: IMPALA-8741: Speed up bit unpacking by vectorisation .. Patch Set 4: (8 comments) I started reviewing this morning but ran out of time to look today. I got through the C++ code but haven't reviewed the guts of the code generator. I have pretty good confidence it's correct though, the unit tests should provide good coverage. http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/benchmarks/bit-packing-benchmark.cc File be/src/benchmarks/bit-packing-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/benchmarks/bit-packing-benchmark.cc@29 PS4, Line 29: // The second one compares the original scalar implementation with the vectorised one Include results for this one as a comment? http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing-vectorized.h File be/src/util/bit-packing-vectorized.h: PS4: We generally don't check in generated files. There are arguments for and against doing so, but generally that's the direction we've gone. The most compelling reason for me is that re-generating the code as part of the build means that vectorised_bit_unpacking_generator.py is tested. Otherwise it could easily bit rot. I think this is useful for the purposes of review, but I'd be inclined to remove it before merging and rely on generating via a CMake rule. We can discuss the pros and cons though; maybe there are some considerations I'm issing. http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.h File be/src/util/bit-packing.h: http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.h@64 PS4, Line 64: simultaniously simultaneously http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.h@67 PS4, Line 67: template Is there a significant performance benefit to making VECTORIZE a compile-time constant - we already have to do a runtime check for the instruction anyway, so it can't result in more specialisation. http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.inline.h@84 PS4, Line 84: if (LIKELY((std::is_same::value Does it even make sense to unpack values into a different type outside of these 4? Could we make this a static_assert instead? That would avoid someone accidentally instantiating a non-vectorized version. http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/bit-packing.inline.h@262 PS4, Line 262: return in; indentation http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/vectorised_bit_unpacking_generator.py File be/src/util/vectorised_bit_unpacking_generator.py: http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/vectorised_bit_unpacking_generator.py@142 PS4, Line 142: sinlge single http://gerrit.cloudera.org:8080/#/c/13807/4/be/src/util/vectorised_bit_unpacking_generator.py@1080 PS4, Line 1080: metod method -- To view, visit http://gerrit.cloudera.org:8080/13807 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e452a547973778bbd8d768c608e1a32e948f947 Gerrit-Change-Number: 13807 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 23:02:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8717: impala-shell support for HS2 HTTP endpoint
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13746 ) Change subject: IMPALA-8717: impala-shell support for HS2 HTTP endpoint .. Patch Set 5: I'm having trouble getting to this, might be a couple of days still. I think Thomas would also be a good reviewer for this if he has any review cycles. -- To view, visit http://gerrit.cloudera.org:8080/13746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8323950857dfe1c1dfd5377fde79f87bc2ce9534 Gerrit-Change-Number: 13746 Gerrit-PatchSet: 5 Gerrit-Owner: Bharath Vissapragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 17 Jul 2019 23:24:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8484: Run queries on disjoint executor groups
Lars Volker has posted comments on this change. (
http://gerrit.cloudera.org:8080/13550 )
Change subject: IMPALA-8484: Run queries on disjoint executor groups
..
Patch Set 10:
(51 comments)
Thanks for the comments. Please see PS10
http://gerrit.cloudera.org:8080/#/c/13550/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13550/9//COMMIT_MSG@20
PS9, Line 20: will it be considered for admission.
> Document the EXECUTOR_GROUP query option?
I removed it altogether.
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/runtime/exec-env.h@268
PS9, Line 268: admit_num_querie
> Maybe admit_num_queries_limit? Probably a matter of taste but it would be m
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/runtime/exec-env.cc@289
PS9, Line 289: n
> Consider making it a constant up the top of the file.
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h
File be/src/scheduling/admission-controller.h:
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@158
PS9, Line 158: .
: /// Each executor group belongs to a single resource
> I found this a little tricky to parse. Here's one alternative phrasing.
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@181
PS9, Line 181: or in the group has an a
> Add parens for consistency, I.e. FindGroupToAdmitAndReject() and DequeueLoo
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@188
PS9, Line 188: groups
> We probably don't need something in this level of detail, but it might be
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@283
PS9, Line 283: ///
> Can you mention the lifetime/ownership of the objects referred to here. Ass
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@592
PS9, Line 592: double wait_time_ms_ema_;
> Can this be NULL? Maybe should be a reference if non-nullable. Or if nullab
It cannot be null, but executor groups are being handed around as pointes in
most places and I followed that pattern here. Changed it to const&.
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@595
PS9, Line 595: void InitMetrics();
> This is mostly a matter of taste, but I have a bit of a bias towards just u
Done.
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@628
PS9, Line 628: /// Structure stored in the RequestQueue representing an
admission request. This struct
> nit: I found it a little tricky to see the delimitation of the groups of me
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@700
PS9, Line 700: /// that the dequeue thread does not need to access the
configs via the request pool
> Document in what cases it might fail?
Done.
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@707
PS9, Line 707: ConditionVariable dequeue_cv_;
> I found the output boolean a little confusing. I don't know if switching th
Changed the polarity. Both feel somewhat awkward to me, but I tried an enum and
that made the calling code much more verbose
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.h@752
PS9, Line 752: e->admitted_schedu
> 'unavailable_reason' for consistency.
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.cc
File be/src/scheduling/admission-controller.cc:
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.cc@509
PS9, Line 509: if (num_admitted >= admit_num_queries_limit) {
> Maybe: num_admitted >= admin_num_limit
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.cc@523
PS9, Line 523: // (a) There are already queued requests (and this is not
admitting from the queue).
> This should be "One of the executors is already at its maximum number of re
Done. I think we do both so I added a 4th condition.
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.cc@561
PS9, Line 561: != nullp
> Probably not?
Done
http://gerrit.cloudera.org:8080/#/c/13550/9/be/src/scheduling/admission-controller.cc@738
PS9, Line 738: // Note the queue_node will not exist in the queue when this
method returns.
> Is this reachable? I spent a couple of minutes trying to understand how we'
This can happen if the local backend is still executing ImpalaServer::Start()
but hasn't set ImpalaServer::services_started_. I added a debug action in the
impala server to confirm this and added a test to test_admissi
[Impala-ASF-CR] IMPALA-8484: Run queries on disjoint executor groups
Hello Andrew Sherman, Michael Ho, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13550 to look at the new patch set (#10). Change subject: IMPALA-8484: Run queries on disjoint executor groups .. IMPALA-8484: Run queries on disjoint executor groups This change adds support for running queries inside a single admission control pool on one of several, disjoint sets of executors called "executor groups". Executors can be configured with an executor group through the newly added '--executor_groups' flag. Note that in anticipation of future changes, the flag already uses the plural form, but only a single executor group may be specified for now. Each executor group specification can optionally contain a minimum size, separated by a ':', e.g. --executor_groups default-pool-1:3. Only when the cluster membership contains at least that number of executors for the groups will it be considered for admission. Executor groups are mapped to resource pools by their name: An executor group can service queries from a resource pool if the pool name is a prefix of the group name separated by a '-'. For example, queries in poll poolA can be serviced by executor groups named poolA-1 and poolA-2, but not by groups name foo or poolB-1. During scheduling, executor groups are considered in alphabetical order. This means that one group is filled up entirely before a subsequent group is considered for admission. Groups also need to pass a health check before considered. In particular, they must contain at least the minimum number of executors specified. If no group is specified during startup, executors are added to the a default executor group. If - during admission - no executor group for a pool can be found and the default group is non-empty, then the default group is considered. The default group does not have a minimum size. This change inverts the order of scheduling and admission. Prior to this change, queries were scheduled before submitting them to the admission controller. Now the admission controller computes schedules for all candidate executor groups before each admission attempt. If the cluster membership has not changed, then the schedules of the previous attempt will be reused. This means that queries will no longer fail if the cluster membership changes while they are queued in the admission controller. Testing: This change adds a new custom cluster test for executor groups. It makes use of new capabilities added to start-impala-cluster.py to bring up additional executors into an already running cluster. Additionally, this change adds an instructional implementation of executor group based autoscaling, which can be used during development. It also adds a helper to run queries concurrently. Both are used in a new test to exercise the executor group logic and to prevent regressions to these tools. In addition to these tests, the existing tests for the admission controller (both BE and EE tests) thoroughly exercise the changed code. Some of them required changes themselves to reflect the new behavior. TODO: Loop the new tests for a day to get some confidence that they're not flaky. Known limitations: When using executor groups, only a single coordinator and a single AC pool are supported. Executors to not include the number of currently running queries in their statestore updates and so admission controllers are not aware of the number of queries admitted by other controllers per host. Change-Id: I8a1d0900f2a82bd2fc0a906cc094e442cffa189b --- M be/src/runtime/coordinator.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/cluster-membership-mgr-test.cc M be/src/scheduling/cluster-membership-mgr.cc M be/src/scheduling/cluster-membership-mgr.h M be/src/scheduling/cluster-membership-test-util.cc M be/src/scheduling/cluster-membership-test-util.h M be/src/scheduling/executor-group-test.cc M be/src/scheduling/executor-group.cc M be/src/scheduling/executor-group.h M be/src/scheduling/query-schedule.cc M be/src/scheduling/query-schedule.h M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h 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 M be/src/service/query-options.h M be/src/util/runtime-profile.h M bin/start-impala-cluster.py M common/thrift/StatestoreService.thrift M tests/common/custom_cluster_test_suite.py M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/common/resource_pool_config.py M tests/custom_cluster/test_admission_controller.py A tests/custom_cluster/test_aut
[Impala-ASF-CR] IMPALA-8484: Run queries on disjoint executor groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13550 ) Change subject: IMPALA-8484: Run queries on disjoint executor groups .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/13550/10/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13550/10/be/src/scheduling/admission-controller.h@187 PS10, Line 187: /// DequeueLoop(). If the cluster membership has changed, it (re-)computes schedules for all line too long (92 > 90) http://gerrit.cloudera.org:8080/#/c/13550/10/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/13550/10/tests/custom_cluster/test_admission_controller.py@1100 PS10, Line 1100: d flake8: E306 expected 1 blank line before a nested definition, found 0 http://gerrit.cloudera.org:8080/#/c/13550/10/tests/custom_cluster/test_admission_controller.py@1102 PS10, Line 1102: = flake8: E225 missing whitespace around operator http://gerrit.cloudera.org:8080/#/c/13550/10/tests/util/concurrent_workload.py File tests/util/concurrent_workload.py: http://gerrit.cloudera.org:8080/#/c/13550/10/tests/util/concurrent_workload.py@81 PS10, Line 81: p flake8: F841 local variable 'period_s' is assigned to but never used -- To view, visit http://gerrit.cloudera.org:8080/13550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a1d0900f2a82bd2fc0a906cc094e442cffa189b Gerrit-Change-Number: 13550 Gerrit-PatchSet: 10 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 18 Jul 2019 00:44:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13776 ) Change subject: IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration .. Patch Set 8: Build Started https://jenkins.impala.io/job/gerrit-docs-auto-test/388/ Testing docs change - this change appears to modify docs/ and no code. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/13776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieec79ac0bbb860c6394a3bf0617b285a7d23ca9e Gerrit-Change-Number: 13776 Gerrit-PatchSet: 8 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Jul 2019 00:46:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration
Hello Thomas Tauber-Marshall, Mike Percy, Andrew Wong, Grant Henke, Hao Hao, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13776 to look at the new patch set (#8). Change subject: IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration .. IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration Change-Id: Ieec79ac0bbb860c6394a3bf0617b285a7d23ca9e --- M docs/shared/impala_common.xml M docs/topics/impala_kudu.xml M docs/topics/impala_tables.xml 3 files changed, 86 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/13776/8 -- To view, visit http://gerrit.cloudera.org:8080/13776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieec79ac0bbb860c6394a3bf0617b285a7d23ca9e Gerrit-Change-Number: 13776 Gerrit-PatchSet: 8 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Tauber-Marshall
[Impala-ASF-CR] [DOCS] An upgrade consideratiobn for IMPALA-7800
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13879 ) Change subject: [DOCS] An upgrade consideratiobn for IMPALA-7800 .. Patch Set 1: Build Started https://jenkins.impala.io/job/gerrit-docs-auto-test/389/ Testing docs change - this change appears to modify docs/ and no code. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/13879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia239fec4973722bc2e53239d3dc5df608a341d92 Gerrit-Change-Number: 13879 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Jul 2019 00:50:34 + Gerrit-HasComments: No
[Impala-ASF-CR] [DOCS] An upgrade consideratiobn for IMPALA-7800
Alex Rodoni has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13879 Change subject: [DOCS] An upgrade consideratiobn for IMPALA-7800 .. [DOCS] An upgrade consideratiobn for IMPALA-7800 Change-Id: Ia239fec4973722bc2e53239d3dc5df608a341d92 --- M docs/topics/impala_upgrading.xml 1 file changed, 19 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/13879/1 -- To view, visit http://gerrit.cloudera.org:8080/13879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia239fec4973722bc2e53239d3dc5df608a341d92 Gerrit-Change-Number: 13879 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni
[Impala-ASF-CR] IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13776 ) Change subject: IMPALA-8727: [DOCS] Impala-side changes for Kudu HMS integration .. Patch Set 8: Verified+1 Build Successful https://jenkins.impala.io/job/gerrit-docs-auto-test/388/ : Doc tests passed. -- To view, visit http://gerrit.cloudera.org:8080/13776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieec79ac0bbb860c6394a3bf0617b285a7d23ca9e Gerrit-Change-Number: 13776 Gerrit-PatchSet: 8 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Comment-Date: Thu, 18 Jul 2019 01:09:29 + Gerrit-HasComments: No
[Impala-ASF-CR] [DOCS] An upgrade consideratiobn for IMPALA-7800
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13879 ) Change subject: [DOCS] An upgrade consideratiobn for IMPALA-7800 .. Patch Set 1: Verified+1 Build Successful https://jenkins.impala.io/job/gerrit-docs-auto-test/389/ : Doc tests passed. -- To view, visit http://gerrit.cloudera.org:8080/13879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia239fec4973722bc2e53239d3dc5df608a341d92 Gerrit-Change-Number: 13879 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Thu, 18 Jul 2019 01:10:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8484: Run queries on disjoint executor groups
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13550 ) Change subject: IMPALA-8484: Run queries on disjoint executor groups .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/3904/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/13550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a1d0900f2a82bd2fc0a906cc094e442cffa189b Gerrit-Change-Number: 13550 Gerrit-PatchSet: 10 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 18 Jul 2019 01:22:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13872 ) Change subject: IMPALA-8766: Change cloud dependencies to use hadoop-cloud-storage .. Patch Set 1: Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4608/ -- To view, visit http://gerrit.cloudera.org:8080/13872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I08f1c36ecf54ac277d99e2d2843163eada732e50 Gerrit-Change-Number: 13872 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Comment-Date: Thu, 18 Jul 2019 01:31:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8636: Implement INSERT for insert-only ACID tables
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/13559 )
Change subject: IMPALA-8636: Implement INSERT for insert-only ACID tables
..
Patch Set 11:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@25
PS11, Line 25: The Backend commits/aborts the transaction by calling the
Frontend via
I think this needs to be updated to explain what is done on the catalog.
http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@34
PS11, Line 34: tests only run with exhaustive exploration strategy)
I'm thinking about how we can test more of the error paths, that seems like the
biggest gap at the moment. It might be worth adding cancellation tests like
TestCancellationSerial.test_cancel_insert() that cancels a query against an
ACID table, and I suppose also one that cancels a select query against an acid
table.
Ideally we would also test some of the other error paths (e.g. HMS returns
error) - I think we'd have to add failpoints for that. But I don't want to
expand the scope of this too much - maybe take a look to see what it would take
to do that. Don't think this is a blocker.
http://gerrit.cloudera.org:8080/#/c/13559/11//COMMIT_MSG@37
PS11, Line 37: * add locks and heartbeats
What are the implications of not having heartbeats? Will long-running queries
against ACID tables have the transaction time out on the HMS?
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/exec/hdfs-table-sink.h@134
PS11, Line 134: /
"base or delta". It was confusing whether it was a path separate or or not
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.h@493
PS11, Line 493: /// Gather and publish all required updates to the metastore
Can you document the postcondition around transactions being committed/aborted?
It seems like we're guaranteeing that DML transactions on partitioned tables
should be committed or aborted when this function returns.
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc@1106
PS11, Line 1106: Status rpc_status
=client.DoRpc(&CatalogServiceClientWrapper::UpdateCatalog,
missing space
http://gerrit.cloudera.org:8080/#/c/13559/11/be/src/service/client-request-state.cc@1107
PS11, Line 1107: catalog_update, &resp);
It would be more explicit to
if (!rpc_status.ok()) {
if (InTransaction()) AbortTransaction();
return rpc_status;
}
Actually could maybe combine the checks to reduce the number of error paths
like:
Status status = client.DoRpc();
if (status.ok()) status = Status(resp.result.status);
if (!status.ok()...) {
if (InTransaction()) AbortTransaction();
LOG(ERROR) << "ERROR Finalizing DML: " << status.GetDetail();
return status;
}
This has a slight behaviour change of logging RPC errors, but that seems like a
reasonable thing to do. This is a bit of a nit-pick, and your code is correct,
but this kind of error handling is easy to introduce bugs into. I think the
current code is ok, will leave it to you to determine if it's an improvement or
not.
http://gerrit.cloudera.org:8080/#/c/13559/11/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/13559/11/common/thrift/Frontend.thrift@361
PS11, Line 361: traget
target
http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@941
PS11, Line 941: //TODO writeIDs may also be loaded in other code paths.
I guess you didn't actually add this comment, is there a JIRA for this?
http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/common/TransactionException.java
File fe/src/main/java/org/apache/impala/common/TransactionException.java:
http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/impala/common/TransactionException.java@21
PS11, Line 21: * Thrown for errors related to ACID transactions.
I feel like this could be a little more specific, e.g. "Thrown for errors that
occur when interacting with ACID transactions, e.g. failures to open, commit,
or abort a transaction". I.e. it wouldn't include something like failing a
query because it uses an unsupported table type or something.
http://gerrit.cloudera.org:8080/#/c/13559/11/fe/src/main/java/org/apache/
[Impala-ASF-CR] IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 ) Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations .. Patch Set 6: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/13873/6/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/6/be/src/exec/buffered-plan-root-sink.h@24 PS6, Line 24: /// PlanRootSink that buffers RowBatches from the 'sender' (fragment) thread. RowBatches : /// are buffered in memory until a memory limit is hit. This is just for the initial implementation only, right ? We kind need to support spilling to disk eventually so we can produce all the possible result rows from the underlying plan tree. As it's described now, it's not completely solving the problem of a client not fetching all result rows. http://gerrit.cloudera.org:8080/#/c/13873/6/be/src/exec/plan-root-sink.h File be/src/exec/plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/6/be/src/exec/plan-root-sink.h@a67 PS6, Line 67: : : : : : : : : nit: these are part of the interfaces of DataSink. It seems clearer to just leave them here as pure virtual functions. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 18 Jul 2019 04:58:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Thu, 18 Jul 2019 05:45:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4609/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 6 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Thu, 18 Jul 2019 05:46:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8750: Fix profile observability tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13851 ) Change subject: IMPALA-8750: Fix profile observability tests .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65a1e81870e808f0e261f8a6097efdcc6903912a Gerrit-Change-Number: 13851 Gerrit-PatchSet: 6 Gerrit-Owner: Tamas Mate Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Comment-Date: Thu, 18 Jul 2019 05:46:15 + Gerrit-HasComments: No
