[Impala-ASF-CR] IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10876 )
Change subject: IMPALA-6223: Gracefully handle malformed 'with' queries in
impala-shell
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10876/2/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/10876/2/shell/impala_shell.py@1147
PS2, Line 1147:
> nit: our convention is to use 2 space indentation
I kinda prefer to still send the query to the Impala parser even on error and
so that the parser can print a useful message.
try:
lexer = shlex.shlex(sqlparse.format(query.query.lstrip(),
strip_comments=True)
.encode('utf-8'), posix=True)
lexer.escapedquotes += "'"
# Because the WITH clause may precede DML or SELECT queries,
# just checking the first token is insufficient.
is_dml = False
tokens = list(lexer)
if filter(self.DML_REGEX.match, tokens): is_dml = True
return self._execute_stmt(query, is_dml=is_dml, print_web_link=True)
except:
return self._execute_stmt(query, print_web_link=True)
[localhost:21000] default> with v as (select 1) select foo(''), ('bar \n;
Query: with v as (select 1) select foo(''), ('bar \n
Query submitted at: 2018-07-05 19:16:51 (Coordinator: http://impala-dev:25000)
ERROR: AnalysisException: Syntax error in line 1:
with v as (select 1) select foo(''), ('bar \n
^
Encountered: Unexpected character
Expected: ADD, ALTER, AND, ARRAY, AS, ASC, BETWEEN, BIGINT, BINARY, BLOCK_SIZE,
BOOLEAN, CACHED, CASCADE, CHANGE, CHAR, COMMENT, COMPRESSION, CROSS, DATE,
DATETIME, DECIMAL, DEFAULT, DESC, DIV, REAL, DROP, ELSE, ENCODING, END, FLOAT,
FOLLOWING, FROM, FULL, GROUP, IGNORE, HAVING, ILIKE, IN, INNER, INTEGER,
IREGEXP, IS, JOIN, LEFT, LIKE, LIMIT, LOCATION, MAP, NOT, NULL, NULLS, OFFSET,
ON, OR, ORDER, PARTITION, PARTITIONED, PRECEDING, PRIMARY, PURGE, RANGE,
RECOVER, REGEXP, RENAME, REPLACE, RESTRICT, RIGHT, RLIKE, ROW, ROWS, SELECT,
SET, SMALLINT, SORT, STORED, STRAIGHT_JOIN, STRING, STRUCT, TABLESAMPLE,
TBLPROPERTIES, THEN, TIMESTAMP, TINYINT, TO, UNCACHED, UNION, USING, VALUES,
VARCHAR, WHEN, WHERE, WITH, COMMA, IDENTIFIER
CAUSED BY: Exception: Syntax error
http://gerrit.cloudera.org:8080/#/c/10876/2/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:
http://gerrit.cloudera.org:8080/#/c/10876/2/tests/shell/test_shell_commandline.py@655
PS2, Line 655:
nit: Python's docstring convention is to not have a space after """, similarly
with the end comment.
--
To view, visit http://gerrit.cloudera.org:8080/10876
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d
Gerrit-Change-Number: 10876
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Comment-Date: Fri, 06 Jul 2018 02:27:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10704 ) Change subject: IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans .. IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans This change ensures that the planner computes parquet conjuncts only when for scans containing parquet files. Additionally, it also handles PARQUET_DICTIONARY_FILTERING and PARQUET_READ_STATISTICS query options in the planner. Testing was carried out independently on parquet and non-parquet scans: 1. Parquet scans were tested via the existing parquet-filtering planner test. Additionally, a new test [parquet-filtering-disabled] was added to ensure that the explain plan generated skips parquet predicates based on the query options. 2. Non-parquet scans were tested manually to ensure that the functions to compute parquet conjucts were not invoked. Additional test cases were added to the parquet-filtering planner test to scan non parquet tables and ensure that the plans do not contain conjuncts based on parquet statistics. 3. A parquet partition was added to the alltypesmixedformat table in the functional database. Planner tests were added to ensure that Parquet conjuncts are constructed only when the Parquet partition is included in the query. Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Reviewed-on: http://gerrit.cloudera.org:8080/10704 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M testdata/bin/create-load-data.sh M testdata/bin/load-dependent-tables.sql M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test A testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test M testdata/workloads/functional-query/queries/QueryTest/mixed-format.test M testdata/workloads/functional-query/queries/QueryTest/show-stats.test M tests/query_test/test_rows_availability.py 14 files changed, 483 insertions(+), 34 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10704 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Gerrit-Change-Number: 10704 Gerrit-PatchSet: 16 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10704 ) Change subject: IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans .. Patch Set 15: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10704 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Gerrit-Change-Number: 10704 Gerrit-PatchSet: 15 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 02:06:49 + Gerrit-HasComments: No
[Impala-ASF-CR] Update Impala to adapt to KRPC changes
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10767 ) Change subject: Update Impala to adapt to KRPC changes .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10767 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I5424b54fbb6ab727c9daeaeea3032384fe6c53be Gerrit-Change-Number: 10767 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-6691: KRPC w/ kerberos fails on SLES11
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10764 ) Change subject: IMPALA-6691: KRPC w/ kerberos fails on SLES11 .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10764 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Icda4173ae0adbc12d167b9918e22b47fd460498c Gerrit-Change-Number: 10764 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] Propagate HAVE PIPE2 compile time value to files that use it
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10761 ) Change subject: Propagate HAVE_PIPE2 compile time value to files that use it .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I72069f32d98e1ba9a2b475344dd5d30f5884464d Gerrit-Change-Number: 10761 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] KUTIL: Various fixes
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10766 ) Change subject: KUTIL: Various fixes .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10766 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ibe259bb155bb00ec50db0299321a0a4338d954bf Gerrit-Change-Number: 10766 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-5873: Check for existence of sync file range()
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10762 ) Change subject: IMPALA-5873: Check for existence of sync_file_range() .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ib4f9409ca3a519b8b9eb5ca91a3c2a6c7c25e34f Gerrit-Change-Number: 10762 Gerrit-PatchSet: 1 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] GUTIL: Rename constant to Kudu's value
Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/10768 ) Change subject: GUTIL: Rename constant to Kudu's value .. Abandoned Not needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/10768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ib62c8e5b1a916e1d6a6c4472ab9b8dda94330115 Gerrit-Change-Number: 10768 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-4669: [KUTIL] Add kudu util library to the build.
Hello Michael Ho, Henry Robinson, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10758 to look at the new patch set (#3). Change subject: IMPALA-4669: [KUTIL] Add kudu_util library to the build. .. IMPALA-4669: [KUTIL] Add kudu_util library to the build. A few miscellaneous changes to allow kudu_util to compile with Impala. Add kudu_version.cc to substitute for the version.cc file that is automatically built during the full Kudu build. Set LZ4_DISABLE_DEPRECATE_WARNINGS to allow Kudu's compressor utility to use deprecated names for LZ4 methods. Add NO_NVM_SUPPORT flag to Kudu build (plan to upstream this later) to disable building with nvm support, removing a library dependency. Also remove imported FindOpenSSL.cmake in favour of the standard one provided by cmake itself. Finally, a few changes to allow compilation on RHEL5: * Only use sched_getcpu() if supported * Only include magic.h if available * Workaround for kernels that don't have SOCK_NONBLOCK * Workaround for kernels that don't have O_CLOEXEC (ignore the flag) * Provide non-working implementation of fallocate() * Disable inclusion of linux/fiemap.h - although this exists on RHEL5, it does not compile due to other #includes in env_posix.cc. We disable the path this is used for, since Impala does not call that code. * Use Kudu's implementation of pipe(2), preadv(2) and pwritev(2) where it doesn't exist. In most cases these changes simply force kutil to revert to a different implementation that was already written for OSX support - this patch generalises the logic to provide the implementation whenever the required function doesn't exist. This patch compiles on RHEL5.5 and 6.0, SLES11 and 12, Ubuntu 12.04 and 14.04 and Debian 7.0 and 8.0. Change-Id: I451f02d3e4669e8a548b92fb1445cb2b322659a2 Reviewed-on: http://gerrit.cloudera.org:8080/5715 Tested-by: Impala Public Jenkins Reviewed-by: Henry Robinson --- M CMakeLists.txt M be/src/kudu/util/CMakeLists.txt M be/src/kudu/util/compression/compression_codec.cc M be/src/kudu/util/env_posix.cc M be/src/kudu/util/flags.cc A be/src/kudu/util/kudu_export.h M be/src/kudu/util/logging.cc M be/src/kudu/util/minidump.cc 8 files changed, 77 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10758/3 -- To view, visit http://gerrit.cloudera.org:8080/10758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I451f02d3e4669e8a548b92fb1445cb2b322659a2 Gerrit-Change-Number: 10758 Gerrit-PatchSet: 3 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-4669: [KRPC] Add kudu rpc library to build
Hello Henry Robinson, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10760 to look at the new patch set (#3). Change subject: IMPALA-4669: [KRPC] Add kudu_rpc library to build .. IMPALA-4669: [KRPC] Add kudu_rpc library to build Import FindKRPC.cmake from Apache Kudu. Add some files to protoc-gen-krpc link to allow it to find symbols now defined within Impala (without linking all of Impala's libraries). Change-Id: I5693288db90f2e9673b8c88ca4378c3790cba957 Reviewed-on: http://gerrit.cloudera.org:8080/5719 Reviewed-by: Henry Robinson Tested-by: Impala Public Jenkins --- M be/src/kudu/rpc/CMakeLists.txt M be/src/rpc/impala-service-pool.cc M be/src/rpc/rpc-mgr-test-base.h M be/src/runtime/krpc-data-stream-mgr.cc 4 files changed, 13 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/10760/3 -- To view, visit http://gerrit.cloudera.org:8080/10760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5693288db90f2e9673b8c88ca4378c3790cba957 Gerrit-Change-Number: 10760 Gerrit-PatchSet: 3 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] Pick parts of recent Kudu gutil changes
Hello Michael Ho, Sailesh Mukil, Joe McDonnell, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10769 to look at the new patch set (#7). Change subject: Pick parts of recent Kudu gutil changes .. Pick parts of recent Kudu gutil changes - Include some ASAN macros from gutil (Kudu commit c8724c61) - Pick parts of KUDU-2427 (Kudu commit b7cf3b2e) - Rename constants These changes will be subsumed by a proper rebase of GUTIL. Change-Id: Id2dc8c70425e3ac030427ebeb1ec18a44d14d5cb --- M be/src/gutil/macros.h M be/src/gutil/port.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/sysinfo.cc M be/src/util/error-util.h 6 files changed, 100 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/10769/7 -- To view, visit http://gerrit.cloudera.org:8080/10769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id2dc8c70425e3ac030427ebeb1ec18a44d14d5cb Gerrit-Change-Number: 10769 Gerrit-PatchSet: 7 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Hello Sailesh Mukil, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10763 to look at the new patch set (#3). Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork .. IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork Impala currently kinits by forking off a child process. This has proved to be expensive in many cases since the subprocess tries to reserve as much memory as Impala is currently using which can be quite a lot. This patch adds a flag called 'use_kudu_kinit' that defaults to true. When it's true, it uses the Kudu security library's kinit code that programatically uses the krb5 library to kinit. When it's false, we run our current path which kicks off the kinit-thread and forks off a kinit process periodically to reacquire tickets based on FLAGS_kerberos_reinit_interval. Converted existing tests in thrift-server-test to run with and without kerberos. We now run this BE test with kerberos by using Kudu's MiniKdc utility. This introduces a new dependency on some kerberos binaries that are checked through FindKerberosPrograms.cmake. Note that this is only a test dependency and not a dependency for the impalad binaries and friends. Compilation will still succeed if the kerberos binaries for the MiniKdc are not found, however, the thrift-server-test will fail. We run with and without the 'use_kudu_kinit' flag. TODO: Since the setting up and tearing down of our security code isn't idempotent, we can run only any one test in a process with Kerberos now (IMPALA-6085). Updated bin/bootstrap_system.sh to install new sasl-gssapi modules and the kerberos binaries required for the MiniKdc. Also fixed a bug that didn't transfer the environment into 'sudo' in bin/bootstrap_system.sh. Testing: Verified with thrift-server-test and also manually on a live kerberized cluster. Change-Id: Ie3c6e933c454e7adca69ef03e7d5c0c84b656895 Reviewed-on: http://gerrit.cloudera.org:8080/7938 Reviewed-by: Sailesh Mukil Tested-by: Impala Public Jenkins --- M be/src/kudu/security/CMakeLists.txt M be/src/kudu/security/test/mini_kdc.cc 2 files changed, 12 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/10763/3 -- To view, visit http://gerrit.cloudera.org:8080/10763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3c6e933c454e7adca69ef03e7d5c0c84b656895 Gerrit-Change-Number: 10763 Gerrit-PatchSet: 3 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4669: [KSECURITY] Add security library to build
Hello Michael Ho, Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10759
to look at the new patch set (#3).
Change subject: IMPALA-4669: [KSECURITY] Add security library to build
..
IMPALA-4669: [KSECURITY] Add security library to build
* Minor compilation fix
* Add krb5 as a non-toolchain dependency
* Handle legacy versions of libkrb5.so by providing implementation of
krb5_is_config_principal().
* Link against openssl from the toolchain if 1.0.0 or higher not found
on build machine.
* Update LICENSE.txt and NOTICE.txt re: OpenSSL code in x509_check_host.{h,c}.
Change-Id: Ifab51d887f5e771ad62eeddc14b9c47f42c3130d
Reviewed-on: http://gerrit.cloudera.org:8080/5717
Reviewed-by: Henry Robinson
Tested-by: Henry Robinson
---
M be/CMakeLists.txt
M be/src/common/config.h.in
M be/src/kudu/security/init.cc
3 files changed, 12 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/10759/3
--
To view, visit http://gerrit.cloudera.org:8080/10759
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifab51d887f5e771ad62eeddc14b9c47f42c3130d
Gerrit-Change-Number: 10759
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
Hello Sailesh Mukil, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10763 to look at the new patch set (#2). Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork .. IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork Impala currently kinits by forking off a child process. This has proved to be expensive in many cases since the subprocess tries to reserve as much memory as Impala is currently using which can be quite a lot. This patch adds a flag called 'use_kudu_kinit' that defaults to true. When it's true, it uses the Kudu security library's kinit code that programatically uses the krb5 library to kinit. When it's false, we run our current path which kicks off the kinit-thread and forks off a kinit process periodically to reacquire tickets based on FLAGS_kerberos_reinit_interval. Converted existing tests in thrift-server-test to run with and without kerberos. We now run this BE test with kerberos by using Kudu's MiniKdc utility. This introduces a new dependency on some kerberos binaries that are checked through FindKerberosPrograms.cmake. Note that this is only a test dependency and not a dependency for the impalad binaries and friends. Compilation will still succeed if the kerberos binaries for the MiniKdc are not found, however, the thrift-server-test will fail. We run with and without the 'use_kudu_kinit' flag. TODO: Since the setting up and tearing down of our security code isn't idempotent, we can run only any one test in a process with Kerberos now (IMPALA-6085). Updated bin/bootstrap_system.sh to install new sasl-gssapi modules and the kerberos binaries required for the MiniKdc. Also fixed a bug that didn't transfer the environment into 'sudo' in bin/bootstrap_system.sh. Testing: Verified with thrift-server-test and also manually on a live kerberized cluster. Change-Id: Ie3c6e933c454e7adca69ef03e7d5c0c84b656895 Reviewed-on: http://gerrit.cloudera.org:8080/7938 Reviewed-by: Sailesh Mukil Tested-by: Impala Public Jenkins --- M be/src/kudu/security/CMakeLists.txt M be/src/kudu/security/test/mini_kdc.cc 2 files changed, 12 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/10763/2 -- To view, visit http://gerrit.cloudera.org:8080/10763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3c6e933c454e7adca69ef03e7d5c0c84b656895 Gerrit-Change-Number: 10763 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4669: [KUTIL] Add kudu util library to the build.
Hello Michael Ho, Henry Robinson, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10758 to look at the new patch set (#2). Change subject: IMPALA-4669: [KUTIL] Add kudu_util library to the build. .. IMPALA-4669: [KUTIL] Add kudu_util library to the build. A few miscellaneous changes to allow kudu_util to compile with Impala. Add kudu_version.cc to substitute for the version.cc file that is automatically built during the full Kudu build. Set LZ4_DISABLE_DEPRECATE_WARNINGS to allow Kudu's compressor utility to use deprecated names for LZ4 methods. Add NO_NVM_SUPPORT flag to Kudu build (plan to upstream this later) to disable building with nvm support, removing a library dependency. Also remove imported FindOpenSSL.cmake in favour of the standard one provided by cmake itself. Finally, a few changes to allow compilation on RHEL5: * Only use sched_getcpu() if supported * Only include magic.h if available * Workaround for kernels that don't have SOCK_NONBLOCK * Workaround for kernels that don't have O_CLOEXEC (ignore the flag) * Provide non-working implementation of fallocate() * Disable inclusion of linux/fiemap.h - although this exists on RHEL5, it does not compile due to other #includes in env_posix.cc. We disable the path this is used for, since Impala does not call that code. * Use Kudu's implementation of pipe(2), preadv(2) and pwritev(2) where it doesn't exist. In most cases these changes simply force kutil to revert to a different implementation that was already written for OSX support - this patch generalises the logic to provide the implementation whenever the required function doesn't exist. This patch compiles on RHEL5.5 and 6.0, SLES11 and 12, Ubuntu 12.04 and 14.04 and Debian 7.0 and 8.0. Change-Id: I451f02d3e4669e8a548b92fb1445cb2b322659a2 Reviewed-on: http://gerrit.cloudera.org:8080/5715 Tested-by: Impala Public Jenkins Reviewed-by: Henry Robinson --- M CMakeLists.txt M be/src/kudu/util/CMakeLists.txt M be/src/kudu/util/compression/compression_codec.cc M be/src/kudu/util/env_posix.cc M be/src/kudu/util/flags.cc A be/src/kudu/util/kudu_export.h M be/src/kudu/util/logging.cc M be/src/kudu/util/minidump.cc 8 files changed, 77 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/10758/2 -- To view, visit http://gerrit.cloudera.org:8080/10758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I451f02d3e4669e8a548b92fb1445cb2b322659a2 Gerrit-Change-Number: 10758 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] Pick parts of recent Kudu gutil changes
Hello Michael Ho, Sailesh Mukil, Joe McDonnell, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10769 to look at the new patch set (#6). Change subject: Pick parts of recent Kudu gutil changes .. Pick parts of recent Kudu gutil changes - Include some ASAN macros from gutil (Kudu commit c8724c61) - Pick parts of KUDU-2427 (Kudu commit b7cf3b2e) - Rename constants These changes will be subsumed by a proper rebase of GUTIL. Change-Id: Id2dc8c70425e3ac030427ebeb1ec18a44d14d5cb --- M be/src/gutil/macros.h M be/src/gutil/port.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/sysinfo.cc M be/src/util/error-util.h 6 files changed, 100 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/10769/6 -- To view, visit http://gerrit.cloudera.org:8080/10769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id2dc8c70425e3ac030427ebeb1ec18a44d14d5cb Gerrit-Change-Number: 10769 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4669: [KRPC] Add kudu rpc library to build
Hello Henry Robinson, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10760 to look at the new patch set (#2). Change subject: IMPALA-4669: [KRPC] Add kudu_rpc library to build .. IMPALA-4669: [KRPC] Add kudu_rpc library to build Import FindKRPC.cmake from Apache Kudu. Add some files to protoc-gen-krpc link to allow it to find symbols now defined within Impala (without linking all of Impala's libraries). Change-Id: I5693288db90f2e9673b8c88ca4378c3790cba957 Reviewed-on: http://gerrit.cloudera.org:8080/5719 Reviewed-by: Henry Robinson Tested-by: Impala Public Jenkins --- M be/src/kudu/rpc/CMakeLists.txt M be/src/rpc/impala-service-pool.cc M be/src/rpc/rpc-mgr-test-base.h M be/src/runtime/krpc-data-stream-mgr.cc 4 files changed, 13 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/10760/2 -- To view, visit http://gerrit.cloudera.org:8080/10760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5693288db90f2e9673b8c88ca4378c3790cba957 Gerrit-Change-Number: 10760 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] Bump toolchain version, include libunwind
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10755 to look at the new patch set (#2). Change subject: Bump toolchain version, include libunwind .. Bump toolchain version, include libunwind Change-Id: I0b26f6a342dd7ba282c3f6c4de93745aff2dd095 --- M CMakeLists.txt M be/CMakeLists.txt M bin/bootstrap_toolchain.py M bin/impala-config.sh A cmake_modules/FindLibUnwind.cmake 5 files changed, 54 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10755/2 -- To view, visit http://gerrit.cloudera.org:8080/10755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0b26f6a342dd7ba282c3f6c4de93745aff2dd095 Gerrit-Change-Number: 10755 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Add KRPC folders from kudu@dc40297046 (WIP)
Lars Volker has uploaded a new patch set (#2). (
http://gerrit.cloudera.org:8080/10757 )
Change subject: Add KRPC folders from kudu@dc40297046 (WIP)
..
Add KRPC folders from kudu@dc40297046 (WIP)
cp -a ~/checkout/kudu/src/kudu/{rpc,util,security} be/src/kudu/
Change-Id: I232db2b4ccf5df9aca87b21dea31bfb2735d1ab7
---
A be/src/kudu/rpc/CMakeLists.txt
A be/src/kudu/rpc/acceptor_pool.cc
A be/src/kudu/rpc/acceptor_pool.h
A be/src/kudu/rpc/blocking_ops.cc
A be/src/kudu/rpc/blocking_ops.h
A be/src/kudu/rpc/client_negotiation.cc
A be/src/kudu/rpc/client_negotiation.h
A be/src/kudu/rpc/connection.cc
A be/src/kudu/rpc/connection.h
A be/src/kudu/rpc/connection_id.cc
A be/src/kudu/rpc/connection_id.h
A be/src/kudu/rpc/constants.cc
A be/src/kudu/rpc/constants.h
A be/src/kudu/rpc/exactly_once_rpc-test.cc
A be/src/kudu/rpc/inbound_call.cc
A be/src/kudu/rpc/inbound_call.h
A be/src/kudu/rpc/messenger.cc
A be/src/kudu/rpc/messenger.h
A be/src/kudu/rpc/mt-rpc-test.cc
A be/src/kudu/rpc/negotiation-test.cc
A be/src/kudu/rpc/negotiation.cc
A be/src/kudu/rpc/negotiation.h
A be/src/kudu/rpc/outbound_call.cc
A be/src/kudu/rpc/outbound_call.h
A be/src/kudu/rpc/periodic-test.cc
A be/src/kudu/rpc/periodic.cc
A be/src/kudu/rpc/periodic.h
A be/src/kudu/rpc/protoc-gen-krpc.cc
A be/src/kudu/rpc/proxy.cc
A be/src/kudu/rpc/proxy.h
A be/src/kudu/rpc/reactor-test.cc
A be/src/kudu/rpc/reactor.cc
A be/src/kudu/rpc/reactor.h
A be/src/kudu/rpc/remote_method.cc
A be/src/kudu/rpc/remote_method.h
A be/src/kudu/rpc/remote_user.cc
A be/src/kudu/rpc/remote_user.h
A be/src/kudu/rpc/request_tracker-test.cc
A be/src/kudu/rpc/request_tracker.cc
A be/src/kudu/rpc/request_tracker.h
A be/src/kudu/rpc/response_callback.h
A be/src/kudu/rpc/result_tracker.cc
A be/src/kudu/rpc/result_tracker.h
A be/src/kudu/rpc/retriable_rpc.h
A be/src/kudu/rpc/rpc-bench.cc
A be/src/kudu/rpc/rpc-test-base.h
A be/src/kudu/rpc/rpc-test.cc
A be/src/kudu/rpc/rpc.cc
A be/src/kudu/rpc/rpc.h
A be/src/kudu/rpc/rpc_context.cc
A be/src/kudu/rpc/rpc_context.h
A be/src/kudu/rpc/rpc_controller.cc
A be/src/kudu/rpc/rpc_controller.h
A be/src/kudu/rpc/rpc_header.proto
A be/src/kudu/rpc/rpc_introspection.proto
A be/src/kudu/rpc/rpc_service.h
A be/src/kudu/rpc/rpc_sidecar.cc
A be/src/kudu/rpc/rpc_sidecar.h
A be/src/kudu/rpc/rpc_stub-test.cc
A be/src/kudu/rpc/rpcz_store.cc
A be/src/kudu/rpc/rpcz_store.h
A be/src/kudu/rpc/rtest.proto
A be/src/kudu/rpc/rtest_diff_package.proto
A be/src/kudu/rpc/sasl_common.cc
A be/src/kudu/rpc/sasl_common.h
A be/src/kudu/rpc/sasl_helper.cc
A be/src/kudu/rpc/sasl_helper.h
A be/src/kudu/rpc/serialization.cc
A be/src/kudu/rpc/serialization.h
A be/src/kudu/rpc/server_negotiation.cc
A be/src/kudu/rpc/server_negotiation.h
A be/src/kudu/rpc/service_if.cc
A be/src/kudu/rpc/service_if.h
A be/src/kudu/rpc/service_pool.cc
A be/src/kudu/rpc/service_pool.h
A be/src/kudu/rpc/service_queue-test.cc
A be/src/kudu/rpc/service_queue.cc
A be/src/kudu/rpc/service_queue.h
A be/src/kudu/rpc/transfer.cc
A be/src/kudu/rpc/transfer.h
A be/src/kudu/rpc/user_credentials.cc
A be/src/kudu/rpc/user_credentials.h
A be/src/kudu/security/CMakeLists.txt
A be/src/kudu/security/ca/cert_management-test.cc
A be/src/kudu/security/ca/cert_management.cc
A be/src/kudu/security/ca/cert_management.h
A be/src/kudu/security/cert-test.cc
A be/src/kudu/security/cert.cc
A be/src/kudu/security/cert.h
A be/src/kudu/security/crypto-test.cc
A be/src/kudu/security/crypto.cc
A be/src/kudu/security/crypto.h
A be/src/kudu/security/init.cc
A be/src/kudu/security/init.h
A be/src/kudu/security/kerberos_util.cc
A be/src/kudu/security/kerberos_util.h
A be/src/kudu/security/krb5_realm_override.cc
A be/src/kudu/security/openssl_util.cc
A be/src/kudu/security/openssl_util.h
A be/src/kudu/security/openssl_util_bio.h
A be/src/kudu/security/security-test-util.cc
A be/src/kudu/security/security-test-util.h
A be/src/kudu/security/security_flags.cc
A be/src/kudu/security/security_flags.h
A be/src/kudu/security/simple_acl.cc
A be/src/kudu/security/simple_acl.h
A be/src/kudu/security/test/mini_kdc-test.cc
A be/src/kudu/security/test/mini_kdc.cc
A be/src/kudu/security/test/mini_kdc.h
A be/src/kudu/security/test/test_certs.cc
A be/src/kudu/security/test/test_certs.h
A be/src/kudu/security/test/test_pass.cc
A be/src/kudu/security/test/test_pass.h
A be/src/kudu/security/tls_context.cc
A be/src/kudu/security/tls_context.h
A be/src/kudu/security/tls_handshake-test.cc
A be/src/kudu/security/tls_handshake.cc
A be/src/kudu/security/tls_handshake.h
A be/src/kudu/security/tls_socket-test.cc
A be/src/kudu/security/tls_socket.cc
A be/src/kudu/security/tls_socket.h
A be/src/kudu/security/token-test.cc
A be/src/kudu/security/token.proto
A be/src/kudu/security/token_signer.cc
A be/src/kudu/security/token_signer.h
A be/src/kudu/security/token_signing_key.cc
A be/src/kudu/security/token_signing_key.h
A be/src/kudu/security/token_verifier.cc
A be/src/kudu/security/token_verifier.h
A
[Impala-ASF-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code
Hello Michael Ho, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10765 to look at the new patch set (#2). Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code .. KUDU-2305: Limit sidecars to INT_MAX and fortify socket code Inspection of the code revealed some other local variables that could overflow with large messages. This patch takes two approaches to eliminate the issues. First, it limits the total size of the messages by limiting the total size of the sidecars to INT_MAX. The total size of the protobuf and header components of the message should be considerably smaller, so limiting the sidecars to INT_MAX eliminates messages that are larger than UINT_MAX. This also means that the sidecar offsets, which are unsigned 32-bit integers, are also safe. Given that FLAGS_rpc_max_message_size is limited to INT_MAX at startup, the receiver would reject any message this large anyway. This also helps with the networking codepath, as any given sidecar will have a size less than INT_MAX, so every Slice that interacts with Writev() is shorter than INT_MAX. Second, even with sidecars limited to INT_MAX, the headers and protobuf parts of the messages mean that certain messages could still exceed INT_MAX. This patch changes some of the sockets codepath to tolerate iovec's that reference more than INT_MAX bytes total. Specifically, it changes Writev()'s nwritten bytes to an int64_t for both TlsSocket and Socket. TlsSocket works because it is sending each Slice individually. The first change limited any given Slice to INT_MAX, so each individual Write() should not be impacted. For Socket, Writev() uses sendmsg(). It should do partial network sends to handle this case. Any Write() call specifies its size with a 32-bit integer, and that will not be impacted by this patch. Testing: - Modified TestRpcSidecarLimits() to verify that sidecars are limited to INT_MAX bytes. - Added a test mode to TestRpcSidecarLimits() where it overrides rpc_max_message_size and sends the maximal message. This verifies that the client send codepath can handle the maximal message. Reviewed-on: http://gerrit.cloudera.org:8080/9601 Reviewed-by: Todd Lipcon Tested-by: Todd Lipcon Changes from Kudu version: - Updated declaration of FLAGS_rpc_max_message_size in rpc-mgr.cc and added a warning not to set it larger than INT_MAX. Change-Id: Id23e518995f2bf2f6bf6b49d5f413f3eaa4e79d1 Reviewed-on: http://gerrit.cloudera.org:8080/9748 Reviewed-by: Michael Ho Tested-by: Impala Public Jenkins --- M be/src/kudu/rpc/transfer.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/10765/2 -- To view, visit http://gerrit.cloudera.org:8080/10765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id23e518995f2bf2f6bf6b49d5f413f3eaa4e79d1 Gerrit-Change-Number: 10765 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10876 ) Change subject: IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10876/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/10876/2/shell/impala_shell.py@1147 PS2, Line 1147: nit: our convention is to use 2 space indentation http://gerrit.cloudera.org:8080/#/c/10876/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/10876/2/tests/shell/test_shell_interactive.py@658 PS2, Line 658: without closing quotation I noticed that shlex also throws a ValueError for "No escaped character". Can you add a test case here for that. The following statement is enough to trigger that error: "with v as (select 1) \;" -- To view, visit http://gerrit.cloudera.org:8080/10876 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d Gerrit-Change-Number: 10876 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Comment-Date: Fri, 06 Jul 2018 01:05:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7251: Fix QueryMaintenance calls in Aggregators
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10871 ) Change subject: IMPALA-7251: Fix QueryMaintenance calls in Aggregators .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10871/1/testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test File testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test: http://gerrit.cloudera.org:8080/#/c/10871/1/testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test@253 PS1, Line 253: select count(distinct concat(cast(l_comment as char(120)), cast(l_comment as char(120)), I think there's still a similar bug in NonGroupingAggregator if the expressions evaluated for the aggregate function accumulate memory, e.g. select min(regexp_replace(l_comment, ".", "x")) from tpch.lineitem I also noticed that the memory accounting in the exec summary is wrong because Aggregator::mem_tracker_'s parent isn't the ExecNodes. -- To view, visit http://gerrit.cloudera.org:8080/10871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4dac2bb0a15cdd7315ee15608bae409c125c82f5 Gerrit-Change-Number: 10871 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 00:54:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/10859 ) Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test .. Patch Set 1: Yeah, those should probably be merged before this change, because our build would get broken. Alternatively, we can merge this change, but disable this test with an xfail. The test can be re-enabled in the IMPALA-7242 patch by whoever fixes the Jira. -- To view, visit http://gerrit.cloudera.org:8080/10859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0 Gerrit-Change-Number: 10859 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 06 Jul 2018 00:45:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7238: Use custom timeout for create unique database
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10862 ) Change subject: IMPALA-7238: Use custom timeout for create unique database .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/10862 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f2beb5bc027a4bb44e854bf1dd8919807a92ea0 Gerrit-Change-Number: 10862 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 00:36:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10873 ) Change subject: IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning .. Patch Set 2: (11 comments) lgtm, just a few nits http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@7 PS2, Line 7: Distributed plan describes coordinator-only nodes as scanning the title should usually describe what this patch does/fixed http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@9 PS2, Line 9: The MembershipCallback function of ImpalaServer class was modified : to only send details of executor nodes to the frontend. Thus, while : generating scan plans, the frontend would not assign fragments to : coordinator-only nodes. The MembershipSnapshot class was renamed to : ExecutorMembershipSnapshot for better semantics. you dont need to describe all the changes in the commit message. Just describing the problem, its high level solution, things like that should be good enough. On the other hand you can make a call on weather some specific change to the code needs to be highlighted in the commit message. http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@16 PS2, Line 16: Added a new custom_cluster test where one impalad is a : coordinator-only node. The test verifies that the scan fragments : are assigned only to the executors. again, you dont need to be verbose while explaining this. You can just write something like: "Added a new custom cluster test to verify the same" http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/frontend.h@48 PS2, Line 48: cluster membership snapshot nit: cluster membership snapshot of executors http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1674 PS2, Line 1674: // Send the hostname and ip_address to the frontend only for executor nodes. nit: can probably incorporate this into the comment on L1669 http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1678 PS2, Line 1678: Increment the number of executor nodes. nit: superfluous comment. http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1678 PS2, Line 1678: We need to track the number of : // executors because there may be multiple impalads running on a single host. we can probably move this comment to the thrift definition of TUpdateMembershipRequest in Frontend.thrift http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@714 PS2, Line 714: // Sent from the impalad BE to FE with the latest cluster membership snapshot resulting : // from the Membership heartbeat. update comment to mention executor membership is only updated. http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@716 PS2, Line 716: TUpdateMembershipRequest would be a pretty big name but just to be consistent, how about we rename this to TUpdateExecutorMembershipRequest http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1153 PS2, Line 1153: cluster nit: executorNodes http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java: http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@28 PS2, Line 28: * Singleton class that represents a snapshot of the Impalad cluster membership. Host update comment to mention cluster membership of only executor nodes -- To view, visit http://gerrit.cloudera.org:8080/10873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8 Gerrit-Change-Number: 10873 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 00:33:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7202: Add width bucket() to the decimal fuzz test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10859 ) Change subject: IMPALA-7202: Add width_bucket() to the decimal fuzz test .. Patch Set 1: I saw you filed some bugs: https://issues.apache.org/jira/browse/IMPALA-7242 https://issues.apache.org/jira/browse/IMPALA-7243 Do those need to be fixed before this is merged? -- To view, visit http://gerrit.cloudera.org:8080/10859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f8a63733ebddc07f46c525ca51ad4794dd0 Gerrit-Change-Number: 10859 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 06 Jul 2018 00:33:35 + Gerrit-HasComments: No
[native-toolchain-CR] IMPALA-7252. Backport rate limiting of fadvise calls for glog
Hello Tianyi Wang, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/10877 to review the following change. Change subject: IMPALA-7252. Backport rate limiting of fadvise calls for glog .. IMPALA-7252. Backport rate limiting of fadvise calls for glog This backports a fix from glog[1] which limits the calling of advise to at most once every 2MB written. Tested that glog builds properly with this fix. [1] https://github.com/google/glog/commit/dacd29679633c9b845708e7015bd2c79367a6ea2 Change-Id: I9f889343ffa458f86c3430b6a8cb2c0d5c14254a --- M buildall.sh A source/glog/glog-0.3.4-patches/0003-rate-limit-calls-to-posix_fadvise.patch 2 files changed, 110 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/77/10877/1 -- To view, visit http://gerrit.cloudera.org:8080/10877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9f889343ffa458f86c3430b6a8cb2c0d5c14254a Gerrit-Change-Number: 10877 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Tianyi Wang
[Impala-ASF-CR] IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10873 )
Change subject: IMPALA-6031: Distributed plan describes coordinator-only nodes
as scanning
..
Patch Set 2:
(7 comments)
Change looks good to me - we discussed the approach earlier. Just have some
cleanup suggestions to make it easier for future people to read.
http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@715
PS2, Line 715: // from the Membership heartbeat.
Briefly mention that it only includes executors?
http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@719
PS2, Line 719: num_executor_nodes
num_executors.
http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java
File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java:
http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@28
PS2, Line 28: * Singleton class that represents a snapshot of the Impalad
cluster membership. Host
Briefly mention that it only contains executors.
http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@34
PS2, Line 34: MembershipSnapshot
ExecutorMembershipSnapshot
http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@49
PS2, Line 49: MembershipSnapshot
ExecutorMembershipSnapshot
http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@54
PS2, Line 54: numExecutorNodes_
Maybe numExecutors_?
http://gerrit.cloudera.org:8080/#/c/10873/2/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/10873/2/tests/custom_cluster/test_coordinators.py@274
PS2, Line 274: "where id NOT IN (0,1,2) and string_col IN
('', '', '', NULL) "\
nit: continuations might not be needed because this is in parentheses
--
To view, visit http://gerrit.cloudera.org:8080/10873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8
Gerrit-Change-Number: 10873
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 06 Jul 2018 00:19:36 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@56 PS7, Line 56: Limitations: > Paused makes it sound like it's not making any progress to me - quiesced ha Actually quiesced is wrong, since it's in the process of quiescing - quiesced is the end state when it's idle. -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 00:13:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@56 PS7, Line 56: Limitations: > Yeah, mentioning them in the Jira might also remind us to include this in t Paused makes it sound like it's not making any progress to me - quiesced has fewer connotations like that. http://gerrit.cloudera.org:8080/#/c/10744/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/9/be/src/service/impala-server.h@124 PS9, Line 124: /// run the shutdown function again to set a shorter deadline. > Can they also increase the timeout, e.g. when they realize they need more t It got confusing about what the semantics of that would be, e.g. does it extend the quiesce period too? It feels a bit weird to me support extending the shutdown deadline but not allow canceling it, since that's probably more useful. -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Jul 2018 00:05:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Hello Michael Ho, Pranay Singh, Lars Volker, Fredy Wijaya, Bikramjeet Vig, Dan
Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10744
to look at the new patch set (#10).
Change subject: IMPALA-1760: Implement shutdown command
..
IMPALA-1760: Implement shutdown command
This allows graceful shutdown of executors and partially graceful
shutdown of coordinators (new operations fail, old operations can
continue).
Details:
* In order to allow future admin commands, this is implemented with
function-like syntax and does not add any reserved words.
* ALL privilege is required on the server
* The coordinator impalad that the client is connected to can be shut
down directly with ":shutdown()".
* Remote shutdown of another impalad is supported, e.g. with
":shutdown('hostname')", so that non-coordinators can be shut down
and for the convenience of the client, which does not have to
connect to the specific impalad. There is no assumption that the
other impalad is registered in the statestore; just that the
coordinator can connect to the other daemon's thrift endpoint.
This simplifies things and allows shutdown in various important
cases, e.g. statestore down.
* The shutdown time limit can be overridden to force a quicker or
slower shutdown.
* If shutting down, a banner is shown on the root debug page.
Workflow:
1. (if a coordinator) clients are prevented from submitting
queries to this coordinator via some out-of-band mechanism,
e.g. load balancer
2. the shutdown process is started via ":shutdown()"
3. a bit is set in the statestore and propagated to coordinators,
which stop scheduling fragment instances on this daemon
(if an executor).
4. the quiesce period (which is ideally set to the AC queueing
delay plus some additional leeway) expires
5. once the daemon is drained (i.e. no fragments, no registered
queries), it shuts itself down.
6. If the daemon does not drain (e.g. rogue clients, long-running
queries), after a longer timeout it will shut down anyway.
What this does:
* Executors can be shut down without causing a service-wide outage
* Shutting down an executor will not disrupt any short-running queries
and will wait for long-running queries up to a threshold.
* Coordinators can be shut down without query failures only if
there is an out-of-band mechanism to prevent submission of more
queries to the shut down coordinator.
* Long running queries or other issues (e.g. stuck fragments) will
slow down but not prevent eventual shutdown.
Limitations:
* The quiesce period needs to be configured to be greater than the
latency of statestore updates + scheduling + admission +
coordinator startup. Otherwise a coordinator may send a
fragment instance to the shutting down impalad. (We could
automate this configuration as a follow-on)
* The quiesce period means a minimum latency for shutdown,
even if the cluster is idle.
* We depend on the statestore detecting the process going down
if queries are still running on that backend when the timeout
expires. This may still be subject to existing problems,
e.g. IMPALA-2990.
Tests:
* Added parser, analysis and authorization tests.
* End-to-end test of shutting down impalads.
* End-to-end test of shutting down then restarting an executor while
queries are running.
* End-to-end test of shutting down a coordinator
- New queries cannot be started on coord, existing queries continue to run
- Exercises various Beeswax and HS2 operations.
Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0
---
M be/src/runtime/backend-client.h
M be/src/runtime/data-stream-test.cc
M be/src/scheduling/scheduler.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/fault-injection-util.h
M be/src/util/default-path-handlers.cc
M common/thrift/Frontend.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/LimitElement.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M tests/common/impala_cluster.py
M tests/common/impala_service.py
M
[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down
Lars Volker has posted comments on this change. (
http://gerrit.cloudera.org:8080/10847 )
Change subject: IMPALA-6271: Impala daemon should log a message when it's being
shut down
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@9
PS2, Line 9: Currently Impalad does not log any message when SIGTERM is sent
to impalad
nit: flush left
http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@11
PS2, Line 11: recieved
nit: typo
http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@12
PS2, Line 12: shutdown
nit: two words
http://gerrit.cloudera.org:8080/#/c/10847/2//COMMIT_MSG@14
PS2, Line 14: Testing:
This change should have an automatic test, possibly in custom-cluster-tests. It
should cover all of impalad, statestored, and catalogd.
http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:
http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@62
PS2, Line 62: static void HandlerTerm(int signum)
nit: formatting ({ on same line). You can run clang-format with the config in
.clang-format.
http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@108
PS2, Line 108: old_action
Do we currently set a handler for SIGTERM elsewhere? If not, we should remove
this.
http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/util/minidump.cc
File be/src/util/minidump.cc:
http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/util/minidump.cc@101
PS2, Line 101: recieved
nit: typo
--
To view, visit http://gerrit.cloudera.org:8080/10847
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Thu, 05 Jul 2018 23:59:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10876 Change subject: IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell .. IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell The change handles the exception thrown by shlex while parsing a malformed 'with' query. The shell redirects the error message to stderr and returns CmdStatus.ERROR. This ensures that the shell does not crash due to malformed requests. This patch was tested by adding to both commandline and interactive shell tests. Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d --- M shell/impala_shell.py M tests/shell/test_shell_commandline.py M tests/shell/test_shell_interactive.py 3 files changed, 24 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/10876/2 -- To view, visit http://gerrit.cloudera.org:8080/10876 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d Gerrit-Change-Number: 10876 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar
[Impala-ASF-CR] IMPALA-7095: clean up scan node profiles
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10810 )
Change subject: IMPALA-7095: clean up scan node profiles
..
Patch Set 7:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/data-source-scan-node.cc@359
PS7, Line 359: COUNTER_ADD(rows_read_counter_, rows_read);
> Any idea why rows_returned_counter_ is updated at line 355 but rows_read_co
COUNTER_SET is idempotent and is safe to call more times. Agree its ok to move
it into this conditional.
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hbase-scan-node.h
File be/src/exec/hbase-scan-node.h:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hbase-scan-node.h@119
PS7, Line 119: time
> wall clock time
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@126
PS7, Line 126: throughput
> nit: adding (bytes/sec) like below in the definition of the field seems he
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@150
PS7, Line 150: WARN_UNUSED_RESULT;
> nit: long line.
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/hdfs-scan-node-base.h@521
PS7, Line 521: //
> nit: ///
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node-base.h
File be/src/exec/kudu-scan-node-base.h:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node-base.h@41
PS7, Line 41:
: virtual Status Prepare(RuntimeState* state) override;
: virtual Status Open(RuntimeState* state) override;
: virtual Status GetNext(RuntimeState* state, RowBatch*
row_batch, bool* eos)
: override = 0;
: protected:
: virtual void DebugString(int indentation_level,
std::stringstream* out) const override;
> WARN_UNUSED_RESULT;
It's not really necessary now that the clang precommit builds detect dropped
statuses. Not sure if it actually has an effect even on GCC since we'll be
generally calling these polymorphically via an ExecNode*
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node.h
File be/src/exec/kudu-scan-node.h:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/kudu-scan-node.h@44
PS7, Line 44: virtual Status Prepare(RuntimeState* state) override;
> WARN_UNUSED_RESULT; Same below .
See comment on KuduScanNodeBase
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h
File be/src/exec/scan-node.h:
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@87
PS7, Line 87: the fragment execution thread
> scanner threads
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@202
PS7, Line 202: used by
> only used by
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@209
PS7, Line 209: class ScannerThreadState {
> I suppose this class is mostly thread-safe as most fields are backed by ato
Good point. I updated comments to clarify which methods were thread-safe.
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@211
PS7, Line 211: state
> Mind elaborating ? This is mostly for initializing the counters, right ?
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@214
PS7, Line 214: state
> Open() creates the row batch queue, right ?
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@255
PS7, Line 255:private:
> nit: blank line before private makes it more legible.
Done
http://gerrit.cloudera.org:8080/#/c/10810/7/be/src/exec/scan-node.h@271
PS7, Line 271: boost::scoped_ptr<
> std::unique_ptr<> Aren't we moving away from boost in the long run ?
I think that's an open question. Using scoped_ptr implies that the pointer
isn't movable so provides some additional information. We document this pattern
here:
https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala
That argument makes sense to me but I don't feel super-strongly. Are we moving
away from it in other areas of the codebase? I might start a dev@ thread to see
how people feel.
--
To view, visit http://gerrit.cloudera.org:8080/10810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77286282d42e7764bfdf94c7ec47cec9d743f787
Gerrit-Change-Number: 10810
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 05 Jul 2018 23:34:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7095: clean up scan node profiles
Hello Michael Ho, Zoltan Borok-Nagy, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10810 to look at the new patch set (#8). Change subject: IMPALA-7095: clean up scan node profiles .. IMPALA-7095: clean up scan node profiles Add counters to scan node implementations where they make sense but were missing (e.g. row batch queue counters for multithread Kudu scans) and remove them where they don't make sense (e.g. scanner thread counters for non-multithreaded scans). Refactors the multithreaded Kudu and HDFS scans to share logic via composition (single inheritance doesn't work for this case), which enables the same set of counters to be maintained with shared code. The row batch queueing and thread tracking is now shared. I looked at combining the logic around 'status_', 'lock_' and 'done_' between the two but the details were different enough that it didn't seem worth abstracting. Adds a PeakScannerThreadConcurrency counter - this answers a common question. Fixes RowsRead for data source scans. Fix some of the comments to be more accurate/useful. Testing: Ran exhaustive tests. Ran various types of scans (HDFS, Kudu, HBase, Data source) and inspected the profile output manually. Change-Id: I77286282d42e7764bfdf94c7ec47cec9d743f787 --- M be/src/exec/data-source-scan-node.cc M be/src/exec/data-source-scan-node.h M be/src/exec/hbase-scan-node.cc M be/src/exec/hbase-scan-node.h M be/src/exec/hbase-table-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/hdfs-scan-node-mt.h M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scan-node.h M be/src/exec/hdfs-text-scanner.cc M be/src/exec/kudu-scan-node-base.h M be/src/exec/kudu-scan-node-mt.cc M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/scan-node.cc M be/src/exec/scan-node.h M be/src/runtime/fragment-instance-state.cc M be/src/util/blocking-queue.h M be/src/util/thread.h 20 files changed, 495 insertions(+), 357 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10810/8 -- To view, visit http://gerrit.cloudera.org:8080/10810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I77286282d42e7764bfdf94c7ec47cec9d743f787 Gerrit-Change-Number: 10810 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10704 ) Change subject: IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans .. Patch Set 15: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2776/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10704 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Gerrit-Change-Number: 10704 Gerrit-PatchSet: 15 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 22:46:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10704 ) Change subject: IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10704 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Gerrit-Change-Number: 10704 Gerrit-PatchSet: 15 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 22:46:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10704 ) Change subject: IMPALA-6625: Skip computing parquet conjuncts for non-Parquet scans .. Patch Set 14: Code-Review+2 Carrying over +2. Build failed earlier due to an old flaky test related to incomplete profiles. -- To view, visit http://gerrit.cloudera.org:8080/10704 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d6c26d42db090c8a15c602f6419ad6399c329e7 Gerrit-Change-Number: 10704 Gerrit-PatchSet: 14 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 22:45:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10873 Change subject: IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning .. IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning The MembershipCallback function of ImpalaServer class was modified to only send details of executor nodes to the frontend. Thus, while generating scan plans, the frontend would not assign fragments to coordinator-only nodes. The MembershipSnapshot class was renamed to ExecutorMembershipSnapshot for better semantics. Testing: Added a new custom_cluster test where one impalad is a coordinator-only node. The test verifies that the scan fragments are assigned only to the executors. Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8 --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M common/thrift/Frontend.thrift M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java R fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M tests/custom_cluster/test_coordinators.py 11 files changed, 82 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/10873/2 -- To view, visit http://gerrit.cloudera.org:8080/10873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8 Gerrit-Change-Number: 10873 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 ) Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc File be/src/exec/grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc@407 PS1, Line 407: SCOPED_TIMER(build_timer_); > Do we have a similar bug here? I'm not sure how expr_results_pool_ gets cle Yep. Submitted this so to get the builds green, and filed a new review: https://gerrit.cloudera.org/#/c/10871/ Since the ExecNode::expr_results_pool_ isn't being used anyways, my suggestion is to keep Aggregator::expr_results_pool_ and then eliminate the calls to QueryMaintenance() in the exec nodes to get rid of the duplication. I don't feel strongly about it, though, happy to implement one of your proposed solutions instead. -- To view, visit http://gerrit.cloudera.org:8080/10863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Gerrit-Change-Number: 10863 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 21:22:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7251: Fix QueryMaintenance calls in Aggregators
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10871 Change subject: IMPALA-7251: Fix QueryMaintenance calls in Aggregators .. IMPALA-7251: Fix QueryMaintenance calls in Aggregators A recent change, IMPALA-110 (part 2), refactored PartitionedAggregationNode into several classes, including a new type 'Aggregator'. During this refactor, code that makes local allocations while evaluating exprs was moved from the ExecNode (now AggregationNode/StreamingAggregationNode) into the Aggregators, but code related to cleaning these allocations up (ie QueryMaintenance()) was not, resulting in some queries using an excessive amount of memory. This patch removes all calls to QueryMaintenance() from the exec nodes and moves them into the Aggregators. Testing: - Added a new test case with a mem limit that fails if the expr allocations aren't released in a timely manner. - Passed a full exhaustive run. Change-Id: I4dac2bb0a15cdd7315ee15608bae409c125c82f5 --- M be/src/exec/aggregation-node.cc M be/src/exec/grouping-aggregator.cc M be/src/exec/streaming-aggregation-node.cc M testdata/workloads/functional-query/queries/QueryTest/spilling-regression-exhaustive.test 4 files changed, 26 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/10871/1 -- To view, visit http://gerrit.cloudera.org:8080/10871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4dac2bb0a15cdd7315ee15608bae409c125c82f5 Gerrit-Change-Number: 10871 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@56 PS7, Line 56: Limitations: > Agree these are useful to note but this commit message is already out of co Yeah, mentioning them in the Jira might also remind us to include this in the docs. I think is_quiesced is better, or maybe is_paused (for the simpler word)? http://gerrit.cloudera.org:8080/#/c/10744/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/9/be/src/service/impala-server.h@124 PS9, Line 124: /// run the shutdown function again to set a shorter deadline. Can they also increase the timeout, e.g. when they realize they need more time? http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@115 PS7, Line 115: e6c00ca5cd67b567eb96c6ecfb26f05 > Probably, the idea here was that an sha1 hash was extremely unlikely to be Make sense to me. The breakpad tests use "jhzvlthd", but keeping the sha1 is fine, too. -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 20:44:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 ) Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Gerrit-Change-Number: 10863 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 20:31:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10863 ) Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming .. IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming A recent change, IMPALA-110 (part 2), refactored the aggregation code. During this refactor, a call to QueryMaintenance() was inadvertantely dropped in the AddBatchStreaming() path, causing test_spilling_regression_exhaustive to fail by hitting the memory limit. Testing: - Ran test_spilling_regression_exhaustive Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Reviewed-on: http://gerrit.cloudera.org:8080/10863 Reviewed-by: Michael Ho Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/exec/grouping-aggregator.cc 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: Michael Ho: Looks good to me, but someone else must approve Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Gerrit-Change-Number: 10863 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. IMPALA-6918: Implement COMMENT ON COLUMN This patch implements updating comment on column table or view. Syntax: -- Add a comment 'comment' into column: db.table_or_view.col COMMENT ON COLUMN db.table_or_view.col IS 'comment' -- Delete a comment from column: db.table_or_view.col COMMENT ON COLUMN db.table_or_view.col IS NULL Testing: - Added new front-end tests - Ran all front-end tests - Added new end-to-end tests - Ran end-to-end DDL tests Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Reviewed-on: http://gerrit.cloudera.org:8080/10754 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M common/thrift/JniCatalog.thrift M fe/src/main/cup/sql-parser.cup A fe/src/main/java/org/apache/impala/analysis/ColumnName.java A fe/src/main/java/org/apache/impala/analysis/CommentOnColumnStmt.java M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M tests/metadata/test_ddl.py M tests/metadata/test_ddl_base.py 11 files changed, 346 insertions(+), 31 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Jul 2018 19:08:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 ) Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2775/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Gerrit-Change-Number: 10863 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 17:17:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 ) Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming .. Patch Set 1: Code-Review+2 I'm ok with this fix to unblock the test, but we should investigate the other issues as a follow-up blocker to make sure we didn't break anything else. -- To view, visit http://gerrit.cloudera.org:8080/10863 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36 Gerrit-Change-Number: 10863 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 17:12:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Jul 2018 15:53:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2774/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Jul 2018 15:53:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7027: Mulitple varchar cast fails with distinct
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10858 )
Change subject: IMPALA-7027: Mulitple varchar cast fails with distinct
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@65
PS1, Line 65: (!(type_ == null ? that.type_ == null : type_ == that.type_))
This condition is not very readable. It can be simplified to
if (type != null && type_ != that.type_) return false;
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java:
http://gerrit.cloudera.org:8080/#/c/10858/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@151
PS1, Line 151: AnalyzesOk("select distinct cast('' as varchar(101)) as a, "
+
Need additional test for "select distinct cast('' as varchar(100)) as a,
cast('' as varchar(101)) as b from functional.alltypes"
--
To view, visit http://gerrit.cloudera.org:8080/10858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fa5890eaa89787645c7d3d2eef976f54a34e7c0
Gerrit-Change-Number: 10858
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Thu, 05 Jul 2018 15:50:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 05 Jul 2018 15:47:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10754 ) Change subject: IMPALA-6918: Implement COMMENT ON COLUMN .. IMPALA-6918: Implement COMMENT ON COLUMN This patch implements updating comment on column table or view. Syntax: -- Add a comment 'comment' into column: db.table_or_view.col COMMENT ON COLUMN db.table_or_view.col IS 'comment' -- Delete a comment from column: db.table_or_view.col COMMENT ON COLUMN db.table_or_view.col IS NULL Testing: - Added new front-end tests - Ran all front-end tests - Added new end-to-end tests - Ran end-to-end DDL tests Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 --- M common/thrift/JniCatalog.thrift M fe/src/main/cup/sql-parser.cup A fe/src/main/java/org/apache/impala/analysis/ColumnName.java A fe/src/main/java/org/apache/impala/analysis/CommentOnColumnStmt.java M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M tests/metadata/test_ddl.py M tests/metadata/test_ddl_base.py 11 files changed, 346 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/10754/8 -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10754 )
Change subject: IMPALA-6918: Implement COMMENT ON COLUMN
..
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG@12
PS6, Line 12: .table_or
> State somewhere that this can be null and define the expected behavior for
Done
http://gerrit.cloudera.org:8080/#/c/10754/6/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/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3611
PS6, Line 3611: }
: applyAlterTable(msTbl, true);
: loadTableMetadata(tbl, newCatalogVersion, false, true,
null);
:
> this is the fourth time this error message and check is done... its bound t
Done
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3636
PS6, Line 3636:
> nit: update
Done
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4027
PS6, Line 4027: new Pair<>("fun
> what is this case adding to the one on L4025?
I meant to type functional.alltypes.id (a table) for L4025. Done.
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py@337
PS6, Line 337: is null".fo
> since the null case follows '' (empty string), how do you know that null ha
Makes sense. Done.
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py
File tests/metadata/test_ddl_base.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py@107
PS6, Line 107: comments = di
> for debugging?
Oops. Removed.
--
To view, visit http://gerrit.cloudera.org:8080/10754
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437
Gerrit-Change-Number: 10754
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 05 Jul 2018 15:42:39 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6918: Implement COMMENT ON COLUMN
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/10754 )
Change subject: IMPALA-6918: Implement COMMENT ON COLUMN
..
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG@12
PS6, Line 12: 'comment'
State somewhere that this can be null and define the expected behavior for this
case.
http://gerrit.cloudera.org:8080/#/c/10754/6/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/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3611
PS6, Line 3611: if (!catalog_.tryLockTable(tbl)) {
: throw new InternalException(String.format("Error altering
table/view %s due to " +
: "lock contention.", tbl.getFullName()));
: }
this is the fourth time this error message and check is done... its bound to
become inconsistent, pls factor it into a separate method.
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3636
PS6, Line 3636: updates
nit: update
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:
http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4027
PS6, Line 4027: new Pair<>("fun
what is this case adding to the one on L4025?
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py@337
PS6, Line 337: is null".fo
since the null case follows '' (empty string), how do you know that null had an
effect vs. had no effect? I think it would be more obvious if the null case was
applied to a column with a non-empty comment.
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py
File tests/metadata/test_ddl_base.py:
http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py@107
PS6, Line 107: print(result)
for debugging?
--
To view, visit http://gerrit.cloudera.org:8080/10754
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437
Gerrit-Change-Number: 10754
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 05 Jul 2018 06:20:51 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Hello Michael Ho, Lars Volker, Fredy Wijaya, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10744
to look at the new patch set (#9).
Change subject: IMPALA-1760: Implement shutdown command
..
IMPALA-1760: Implement shutdown command
This allows graceful shutdown of executors and partially graceful
shutdown of coordinators (new operations fail, old operations can
continue).
Details:
* In order to allow future admin commands, this is implemented with
function-like syntax and does not add any reserved words.
* ALL privilege is required on the server
* The coordinator impalad that the client is connected to can be shut
down directly with ":shutdown()".
* Remote shutdown of another impalad is supported, e.g. with
":shutdown('hostname')", so that non-coordinators can be shut down
and for the convenience of the client, which does not have to
connect to the specific impalad. There is no assumption that the
other impalad is registered in the statestore; just that the
coordinator can connect to the other daemon's thrift endpoint.
This simplifies things and allows shutdown in various important
cases, e.g. statestore down.
* The shutdown time limit can be overridden to force a quicker or
slower shutdown.
* If shutting down, a banner is shown on the root debug page.
Workflow:
1. (if a coordinator) clients are prevented from submitting
queries to this coordinator via some out-of-band mechanism,
e.g. load balancer
2. the shutdown process is started via ":shutdown()"
3. a bit is set in the statestore and propagated to coordinators,
which stop scheduling fragment instances on this daemon
(if an executor).
4. the quiesce period (which is ideally set to the AC queueing
delay plus some additional leeway) expires
5. once the daemon is drained (i.e. no fragments, no registered
queries), it shuts itself down.
6. If the daemon does not drain (e.g. rogue clients, long-running
queries), after a longer timeout it will shut down anyway.
What this does:
* Executors can be shut down without causing a service-wide outage
* Shutting down an executor will not disrupt any short-running queries
and will wait for long-running queries up to a threshold.
* Coordinators can be shut down without query failures only if
there is an out-of-band mechanism to prevent submission of more
queries to the shut down coordinator.
* Long running queries or other issues (e.g. stuck fragments) will
slow down but not prevent eventual shutdown.
Limitations:
* The quiesce period needs to be configured to be greater than the
latency of statestore updates + scheduling + admission +
coordinator startup. Otherwise a coordinator may send a
fragment instance to the shutting down impalad. (We could
automate this configuration as a follow-on)
* The quiesce period means a minimum latency for shutdown,
even if the cluster is idle.
* We depend on the statestore detecting the process going down
if queries are still running on that backend when the timeout
expires. This may still be subject to existing problems,
e.g. IMPALA-2990.
Tests:
* Added parser, analysis and authorization tests.
* End-to-end test of shutting down impalads.
* End-to-end test of shutting down then restarting an executor while
queries are running.
* End-to-end test of shutting down a coordinator
- New queries cannot be started on coord, existing queries continue to run
- Exercises various Beeswax and HS2 operations.
Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0
---
M be/src/runtime/backend-client.h
M be/src/runtime/data-stream-test.cc
M be/src/scheduling/scheduler.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-internal-service.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/testutil/fault-injection-util.h
M be/src/util/default-path-handlers.cc
M common/thrift/Frontend.thrift
M common/thrift/ImpalaInternalService.thrift
M common/thrift/StatestoreService.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/LimitElement.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M tests/common/impala_cluster.py
M tests/common/impala_service.py
M
[Impala-ASF-CR] IMPALA-1760: Implement shutdown command
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 ) Change subject: IMPALA-1760: Implement shutdown command .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@46 PS7, Line 46: does > extra word? Done http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc@626 PS7, Line 626: RETURN_IF_ERROR(client_status); > Should we retry if we get a stale client (similar to here: https://github.c Done -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Jul 2018 06:10:02 + Gerrit-HasComments: Yes
