[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 ) Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork .. Patch Set 8: Code-Review+1 (1 comment) Conditional on the rebase on top of https://gerrit.cloudera.org/#/c/8247/ http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG@7 PS8, Line 7: KRPC Kudu -- To view, visit http://gerrit.cloudera.org:8080/7938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7 Gerrit-Change-Number: 7938 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sat, 14 Oct 2017 01:57:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7938 ) Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc File be/src/rpc/thrift-server-test.cc: http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@247 PS7, Line 247: } > This makes the test infra quite flakey. I tried for quite some time, but it OK. That's fine as long as we eventually will get back to this. -- To view, visit http://gerrit.cloudera.org:8080/7938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7 Gerrit-Change-Number: 7938 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sat, 14 Oct 2017 01:10:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Allow the SASL protocol service name to be configurable
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8230 ) Change subject: Allow the SASL protocol service name to be configurable .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc File be/src/kudu/rpc/client_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc@111 PS1, Line 111: const boost::optional& authn_token, > That patch is a tidy cleanup that adds the 'modernize-pass-by-value' rule, It seems better to also cherry-pick that particular commit so we avoid the chances of other conflicts in the future. That commit seems to touch some number of files under rpc/ security/ and util/. -- To view, visit http://gerrit.cloudera.org:8080/8230 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e Gerrit-Change-Number: 8230 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sat, 14 Oct 2017 01:04:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Sat, 14 Oct 2017 01:02:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. IMPALA-6049: breakpad tests: skip all tests with local filesystem The breakpad tests were recently refactored to support inclusion of one of them as a core test. In this refactor, we neglected to ensure setup_class() called its parent. This means the skipping called in said parent doesn't occur, and the test is executed in an unsupported environment (local filesystem). This patch fixes that by ensuring we call the parent setup_class() via super(). Testing: $ TARGET_FILESYSTEM="local" impala-py.test tests/custom_cluster/test_breakpad.py \ -k test_abort_writes_minidump tests/custom_cluster/test_breakpad.py::TestBreakpadCore::test_abort_writes_minidump SKIPPED Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Reviewed-on: http://gerrit.cloudera.org:8080/8272 Reviewed-by: Sailesh Mukil Reviewed-by: Lars Volker Tested-by: Impala Public Jenkins --- M tests/custom_cluster/test_breakpad.py 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: Sailesh Mukil: Looks good to me, but someone else must approve Lars Volker: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
Hello Philip Zeyliger, Joe McDonnell, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8274 to look at the new patch set (#2). Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ .. IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ Hadoop changed behavior regarding encrypted partitions and started automatically provisioning .Trash directories in encrypted partitions. This interplays poorly with the current test. Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 --- M tests/metadata/test_hdfs_encryption.py 1 file changed, 28 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/8274/2 -- To view, visit http://gerrit.cloudera.org:8080/8274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 Gerrit-Change-Number: 8274 Gerrit-PatchSet: 2 Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8085 ) Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet .. Patch Set 8: Code-Review+1 rebase, carry +1 from alex -- To view, visit http://gerrit.cloudera.org:8080/8085 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2 Gerrit-Change-Number: 8085 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Oct 2017 23:10:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/8274 ) Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py File tests/metadata/test_hdfs_encryption.py: http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py@191 PS1, Line 191: # exists. This behavior is expected due to the difference in encryption zones > "the difference in encryption zones between the .Trash and the warehouse di Done http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py@198 PS1, Line 198: # New HDFS behavior succeeds the query and creates trash; the partition removal > s/New HDFS/HDFS 2.8+/? Done -- To view, visit http://gerrit.cloudera.org:8080/8274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 Gerrit-Change-Number: 8274 Gerrit-PatchSet: 1 Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Zach Amsden Gerrit-Comment-Date: Fri, 13 Oct 2017 23:10:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8034 ) Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@251 PS8, Line 251: ReleaseBytes(); > Checking the dict_bytes_cnt_ to be zero seems to be tricky say even if it i I agree with Joe's comment. All codepaths should call Close(), so we can just add a DCHECK here to enforce that they did so. -- To view, visit http://gerrit.cloudera.org:8080/8034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299 Gerrit-Change-Number: 8034 Gerrit-PatchSet: 8 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Oct 2017 22:50:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/8034 )
Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
..
Patch Set 8:
(4 comments)
I took a look at it and just wanted to point out some cases where the new code
is diverging from best practices in our codebase.
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@348
PS8, Line 348: CloseAndUnregisterFromParent
> I found that MemTracker::Close() is not unregistering with the parent MemTr
We should be using Close() here instead of unregistering every tracker (this is
documented in the comment for CloseAndUnregisterFromParent()).
That may be moot since I don't think we need a special MemTracker for this -
it's fine if we just track this against the scan node tracker. There are
pros/cons of tracking things at a finer granularity but generally we don't
track memory at a very fine granularity. Looking at the JIRA description, I can
see it being read as "track the memory against a new MemTracker" but that
wasn't what I intended.
As-is the behaviour added by this patch is particularly weird because we don't
have per-scanner MemTrackers, so you'll end up with tens of these "dict
decoder" trackers hanging off the scan node.
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@133
PS8, Line 133: inline
nit: inline here and nearby isn't necessary for a couple of reason. First, the
compiler can generally figure out that trivial functions can be inlined with or
without a hint. Second, functions defined in C++ class bodies have an implicit
inline hint implied anyway:
https://isocpp.org/wiki/faq/inline-functions#inline-member-fns-more
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@184
PS8, Line 184: if (dict_bytes_cnt_ != 0) {
Nit: check isn't necessary - Release() does this for you.
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@194
PS8, Line 194: // TODO: AnyLimitExceeded() can be called to check if memory
limit has been exceeded.
The TODO should be to use TryConsume(). The asynchronous checks are not the
direction we want to take things in. Michael Ho had a nice comment here
explaining the direction we're trying to take things:
https://issues.apache.org/jira/browse/IMPALA-2399
--
To view, visit http://gerrit.cloudera.org:8080/8034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Number: 8034
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 13 Oct 2017 22:49:17 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections until lcoal catalog initialized.
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8275 Change subject: IMPALA-4704: Disallow client connections until lcoal catalog initialized. .. IMPALA-4704: Disallow client connections until lcoal catalog initialized. Currently, impalad starts beeswax and hs2 servers even if the catalog has not yet been received. As a result, client connections see an error message stating that the impalad is not yet ready. This patch changes the impalad startup sequence to wait until the catalog is received before opening beeswax and hs2 ports and starting their servers. Testing: - python e2e tests that start a cluster without a catalog and check that client connections are rejected as expected. Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6 ok Change-Id: I19da0c751348accab271aea99b2162218e5ed1fb --- M be/src/service/frontend.cc M be/src/service/frontend.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M bin/start-impala-cluster.py M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/service/FrontendTest.java M tests/common/custom_cluster_test_suite.py M tests/common/impala_cluster.py M tests/common/impala_service.py A tests/custom_cluster/test_catalog_wait.py M tests/custom_cluster/test_coordinators.py 13 files changed, 160 insertions(+), 55 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/8275/1 -- To view, visit http://gerrit.cloudera.org:8080/8275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I19da0c751348accab271aea99b2162218e5ed1fb Gerrit-Change-Number: 8275 Gerrit-PatchSet: 1 Gerrit-Owner: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/8202 )
Change subject: IMPALA-4704: Disallow client connections to imapalad until
catalog is received.
..
Patch Set 7:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG@7
PS7, Line 7: imapalad
> nit: typo
reworded.
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@81
PS7, Line 81: action="store_true", help="Starts all cluster
processes except catalogd.")
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@254
PS7, Line 254: catalog_needs_wait
> I think that function tests if the catalog is ready given that we haven't d
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@323
PS7, Line 323: def wait_for_client(impalad,
timeout_in_seconds=CLUSTER_WAIT_TIMEOUT_IN_SECONDS):
> Add a function comment.
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@332
PS7, Line 332: ready = True
> You can just break and use client_hs2 or client_beeswax in the check in L33
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@336
PS7, Line 336: client_beeswax
> hs2_client doesn't have a close() function?
nope. perhaps there is an api I've overlooked. fwict, you'd have to provide a
close request for a given session handle (see hs2_test_suite). at this point,
a port has been opened, but I don't see a close for that one... perhaps the
idea is that you need to keep track of both the port and the client.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@859
PS7, Line 859: ready
> Maybe also mention what "ready" means, i.e. receive the first update from t
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@867
PS7, Line 867: if (getCatalog().isReady()) return;
> It may make sense to add a log message here to indicate that the catalog is
now keeping track of total time- so that's what's printed here.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@868
PS7, Line 868: catalog to be ready
> That phrase kind of suggests that the catalog server is not ready. Maybe be
went with initialized.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@899
PS7, Line 899: support when the catalog is not ready.
> Maybe "is not supported if the local catalog cache is not initialized."
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@587
PS7, Line 587: public void waitForCatalog() {
: frontend_.waitForCatalog();
: }
> single line
Done
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java@117
PS7, Line 117: private void testCatalogIsNotReady(String stmt, Frontend fe) {
: TQueryCtx queryCtx = TestUtils.createQueryContext(
: Catalog.DEFAULT_DB, AuthorizationTest.USER.getName());
: queryCtx.client_request.setStmt(stmt);
: try {
: fe.createExecRequest(queryCtx, new StringBuilder());
: fail("Expected failure to due uninitialized catalog.");
: } catch (IllegalStateException e) {
: assertEquals("Analyzing a query is not support when the
catalog is not ready.",
: e.getMessage());
: } catch (Exception e) {
: fail("Failed to create exec request due to: " +
ExceptionUtils.getStackTrace(e));
: }
: }
> Does this even make sense to keep given the introduced behavior in this pat
Agreed, the precondition should be enough to show what is expected.
http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py@138
PS7, Line 138: # The number of statestore subscribers is cluster_size (# of
impalad) + 1 (for catalogd
[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8274 ) Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ .. Patch Set 1: Code-Review+1 (3 comments) This looks fine to me, with minor comment nits. http://gerrit.cloudera.org:8080/#/c/8274/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8274/1//COMMIT_MSG@13 PS1, Line 13: N.B. this is probably worth fixing upstream. We are upstream! http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py File tests/metadata/test_hdfs_encryption.py: http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py@191 PS1, Line 191: # exists. This behavior is expected due to the difference in encryption zones "the difference in encryption zones between the .Trash and the warehouse directory" If that's true, could we elaborate on "difference in encryption zones" perhaps as suggested here? http://gerrit.cloudera.org:8080/#/c/8274/1/tests/metadata/test_hdfs_encryption.py@198 PS1, Line 198: # New HDFS behavior succeeds the query and creates trash; the partition removal s/New HDFS/HDFS 2.8+/? -- To view, visit http://gerrit.cloudera.org:8080/8274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 Gerrit-Change-Number: 8274 Gerrit-PatchSet: 1 Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 13 Oct 2017 22:07:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+
Zach Amsden has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8274 Change subject: IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ .. IMPALA-6055: Fix hdfs encryption test far Hadoop 2.8+ Hadoop changed behavior regarding encrypted partitions and started automatically provisioning .Trash directories in encrypted partitions. This interplays poorly with the current test. N.B. this is probably worth fixing upstream. Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 --- M tests/metadata/test_hdfs_encryption.py 1 file changed, 27 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/8274/1 -- To view, visit http://gerrit.cloudera.org:8080/8274 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I30234aa50fea93f316e75beea2ced002dcea0c24 Gerrit-Change-Number: 8274 Gerrit-PatchSet: 1 Gerrit-Owner: Zach Amsden
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1336/ -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 13 Oct 2017 21:06:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
Hello Joe McDonnell, Bikramjeet Vig, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8034 to look at the new patch set (#9). Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder .. IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder Currently DictDecoder class and DictEncoder class uses std::vector to store the tables mapping codeword to value and vice-versa. It is hard to detect the memory usage by these tables when they becomes very large, since this memory is not accounted by Impala's memory mangement infrastructure. This patch introduces memory tracker to track the memory used by dictionary used in DictDecoder class and DictEncoder class when a parquet file is read from or written to. Memory tracker is introduced at the class HdfsParquetScanner level to track all the memory used by dictonary in DictDecoders. Similarly memory tracker is introduced in class HdfsParquetTableWriter to track the memory used by dictionary in DictEncoders. Memory for the dictionary, stored as std::vector is still allocated from std:allocator but the amount allocated is accounted by introducing a counter which is incremented and decremented as the memory is consumed and released by vector. Testing --- Ran all the backend and end-end tests with no failures. Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299 --- M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-parquet-scanner.h M be/src/exec/hdfs-parquet-table-writer.cc M be/src/exec/hdfs-parquet-table-writer.h M be/src/exec/parquet-column-readers.cc M be/src/exec/parquet-column-readers.h M be/src/util/dict-encoding.h M be/src/util/dict-test.cc 8 files changed, 170 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8034/9 -- To view, visit http://gerrit.cloudera.org:8080/8034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299 Gerrit-Change-Number: 8034 Gerrit-PatchSet: 9 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pranay Singh
[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
Pranay Singh has posted comments on this change. (
http://gerrit.cloudera.org:8080/8034 )
Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
..
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@274
PS8, Line 274: // Before closing the Column readers, the accounted bytes in
mem_tracker for
: // dict_decoder_ is released.
: if (mem_tracker_ != nullptr) {
: for (ParquetColumnReader* col_reader : column_readers_)
col_reader->Cleanup();
: }
> I think this code won't be necessary if col_reader->Close() does the dictio
Removed this code and wrapped it up with the col_reader->Close()
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-scanner.cc@348
PS8, Line 348: CloseAndUnregisterFromParent
> Same question here as elsewhere: Do we really need CloseAndUnregisterFromPa
I found that MemTracker::Close() is not unregistering with the parent
MemTracker , so I found CloseAndUnregisterFromParent to be more appropriate.
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-table-writer.cc
File be/src/exec/hdfs-parquet-table-writer.cc:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-table-writer.cc@178
PS8, Line 178: dict_encoder_base_->ClearIndices();
> This should call the new base Close() function (which should do ClearIndice
Done
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/hdfs-parquet-table-writer.cc@1057
PS8, Line 1057: CloseAndUnregisterFromParent();
> I think we want to limit use of CloseAndUnregisterFromParent() to cases tha
I checked the MemTracker::Close(), it does not unregister the mem_tracker_ from
it's parent,
Since this mem_tracker_ should not be used beyond this point I have reset it
to NULL
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/parquet-column-readers.h
File be/src/exec/parquet-column-readers.h:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/parquet-column-readers.h@234
PS8, Line 234: /// Delete the bytes used for memory tracking.
: virtual void Cleanup() {}
> I think this can be folded into Close() and removed.
Done
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/exec/parquet-column-readers.cc@269
PS8, Line 269: virtual void Cleanup() {
: dict_decoder_.Close();
: }
> I think this can be folded into Close() without a separate Cleanup() functi
Done
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@107
PS8, Line 107: DictEncoder(MemPool* pool, int encoded_value_size, MemTracker*
mem_tracker) :
: DictEncoderBase(pool), buckets_(HASH_TABLE_SIZE,
Node::INVALID_INDEX),
: encoded_value_size_(encoded_value_size),
: dict_mem_tracker_(mem_tracker),
: dict_bytes_cnt_(0) { }
> As I understand it, the DictEncoderBase/DictEncoder split is that DictEncod
Moved the functionality to the base class as you suggested
http://gerrit.cloudera.org:8080/#/c/8034/8/be/src/util/dict-encoding.h@251
PS8, Line 251: ReleaseBytes();
> Is there any codepath that should legitimately use this? If not, DCHECK tha
Checking the dict_bytes_cnt_ to be zero seems to be tricky say even if it is
not used in a function it can have a non zero value.
--
To view, visit http://gerrit.cloudera.org:8080/8034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Number: 8034
Gerrit-PatchSet: 8
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Pranay Singh
Gerrit-Comment-Date: Fri, 13 Oct 2017 20:45:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8170 ) Change subject: IMPALA-5789: Add always_false flag in bloom filter .. Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/8170/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8170/4//COMMIT_MSG@15 PS4, Line 15: Did you do any perf runs? It would be good to verify that the extra flag checking doesn't affect perf (I suspect it doesn't). It would also be good to confirm that testdata/workloads/targeted-perf/queries/primitive_empty_build_join_1.test gets faster (I think it should!). http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/exec/hdfs-parquet-scanner.cc@403 PS4, Line 403: std:: Shouldn't need std:: - it's imported in common/names.h http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/exec/hdfs-scanner.cc@107 PS4, Line 107: if (!BaseSequenceScanner::FileFormatIsSequenceBased( Might be more readable to factor subexpression into variable e.g. is_sequence_based. http://gerrit.cloudera.org:8080/#/c/8170/4/tests/custom_cluster/test_always_false_filter.py File tests/custom_cluster/test_always_false_filter.py: http://gerrit.cloudera.org:8080/#/c/8170/4/tests/custom_cluster/test_always_false_filter.py@53 PS4, Line 53: seq_table_suffixes = ['_avro', '_rc', '_seq'] We'd normally create a test matrix based on these. Is the idea here to avoid restarting the cluster for each file format? If so it would be good to leave a comment so that readers understand why. http://gerrit.cloudera.org:8080/#/c/8170/4/tests/custom_cluster/test_always_false_filter.py@82 PS4, Line 82: def test_skip_file(self, cursor): Does this need to be a custom cluster test? I.e. does it need a special minicluster to execute. It's best to make things query tests if possible since starting a cluster is slow and the tests aren't parallelisable. -- To view, visit http://gerrit.cloudera.org:8080/8170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d Gerrit-Change-Number: 8170 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Oct 2017 20:34:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4704: Disallow client connections to imapalad until catalog is received.
Dimitris Tsirogiannis has posted comments on this change. (
http://gerrit.cloudera.org:8080/8202 )
Change subject: IMPALA-4704: Disallow client connections to imapalad until
catalog is received.
..
Patch Set 7:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG@7
PS7, Line 7: imapalad
nit: typo
http://gerrit.cloudera.org:8080/#/c/8202/7//COMMIT_MSG@7
PS7, Line 7: catalog
catalog update
http://gerrit.cloudera.org:8080/#/c/8202/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8202/7/be/src/service/impala-server.cc@a2045
PS7, Line 2045:
:) nice
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@81
PS7, Line 81: action="store_true", help="Starts all cluster
processes except catalogd.")
nit: long line
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@254
PS7, Line 254: catalog_needs_wait
I think that function tests if the catalog is ready given that we haven't
disabled it. So maybe "is_catalog_ready" is a better name. Thoughts?
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@323
PS7, Line 323: def wait_for_client(impalad,
timeout_in_seconds=CLUSTER_WAIT_TIMEOUT_IN_SECONDS):
Add a function comment.
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@332
PS7, Line 332: ready = True
You can just break and use client_hs2 or client_beeswax in the check in L339.
http://gerrit.cloudera.org:8080/#/c/8202/7/bin/start-impala-cluster.py@336
PS7, Line 336: client_beeswax
hs2_client doesn't have a close() function?
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@859
PS7, Line 859: ready
Maybe also mention what "ready" means, i.e. receive the first update from the
catalog.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@867
PS7, Line 867: if (getCatalog().isReady()) return;
It may make sense to add a log message here to indicate that the catalog is now
ready after waiting for MAX_CATALOG_UPDATE_WAIT_TIME_MS * num_triews msec.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@868
PS7, Line 868: catalog to be ready
That phrase kind of suggests that the catalog server is not ready. Maybe best
to say that we're waiting for the initial catalog update from the statestore or
for the local catalog cache to be initialized.
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/Frontend.java@899
PS7, Line 899: support when the catalog is not ready.
Maybe "is not supported if the local catalog cache is not initialized."
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@587
PS7, Line 587: public void waitForCatalog() {
: frontend_.waitForCatalog();
: }
single line
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:
http://gerrit.cloudera.org:8080/#/c/8202/7/fe/src/test/java/org/apache/impala/service/FrontendTest.java@117
PS7, Line 117: private void testCatalogIsNotReady(String stmt, Frontend fe) {
: TQueryCtx queryCtx = TestUtils.createQueryContext(
: Catalog.DEFAULT_DB, AuthorizationTest.USER.getName());
: queryCtx.client_request.setStmt(stmt);
: try {
: fe.createExecRequest(queryCtx, new StringBuilder());
: fail("Expected failure to due uninitialized catalog.");
: } catch (IllegalStateException e) {
: assertEquals("Analyzing a query is not support when the
catalog is not ready.",
: e.getMessage());
: } catch (Exception e) {
: fail("Failed to create exec request due to: " +
ExceptionUtils.getStackTrace(e));
: }
: }
Does this even make sense to keep given the introduced behavior in this patch?
http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/8202/7/tests/common/custom_cluster_test_suite.py@138
PS7, Line 138:
[Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8170 ) Change subject: IMPALA-5789: Add always_false flag in bloom filter .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/runtime/coordinator.cc@a20 PS4, Line 20: > Thanks for doing cleanup here. How did you determine which includes were ne I used the Clion ide. http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/runtime/runtime-filter-ir.cc File be/src/runtime/runtime-filter-ir.cc: http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/runtime/runtime-filter-ir.cc@27 PS4, Line 27: uint32_t h = RawValue::GetHashValue(val, col_type, > Won't these branches be likely in some cases? IMO best to only use these an It's removed. http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/8170/4/be/src/util/bloom-filter.h@200 PS4, Line 200: if (always_false_) return false; > Doesn't this mean that RuntimeFilter::Eval() checks always_false_ twice? Ma Removed the other one. -- To view, visit http://gerrit.cloudera.org:8080/8170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d Gerrit-Change-Number: 8170 Gerrit-PatchSet: 6 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Oct 2017 19:12:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter
Hello Thomas Tauber-Marshall, Sailesh Mukil, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8170 to look at the new patch set (#6). Change subject: IMPALA-5789: Add always_false flag in bloom filter .. IMPALA-5789: Add always_false flag in bloom filter This patch adds an always_false flag in bloom filters. The flag is set if nothing has been inserted into the bloom filter. HdfsScanner uses this flag to early terminate the scan at file and split granularities. Testing: It passes existing tests. A test case is added checking that an always-false runtime filter can filter out files and splits. Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d --- M be/src/common/global-flags.cc M be/src/exec/base-sequence-scanner.cc M be/src/exec/base-sequence-scanner.h M be/src/exec/filter-context.cc M be/src/exec/filter-context.h M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scanner.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M be/src/runtime/runtime-filter-ir.cc M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-filter.inline.h M be/src/util/bloom-filter-ir.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/ImpalaInternalService.thrift A tests/custom_cluster/test_always_false_filter.py 21 files changed, 227 insertions(+), 197 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8170/6 -- To view, visit http://gerrit.cloudera.org:8080/8170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d Gerrit-Change-Number: 8170 Gerrit-PatchSet: 6 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter
Hello Thomas Tauber-Marshall, Sailesh Mukil, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8170 to look at the new patch set (#5). Change subject: IMPALA-5789: Add always_false flag in bloom filter .. IMPALA-5789: Add always_false flag in bloom filter This patch adds an always_false flag in bloom filters. The flag is set if nothing has been inserted into the bloom filter. HdfsScanner uses this flag to early terminate the scan at file and split granularities. Testing: It passes existing tests. A test case is added checking that an always-false runtime filter can filter out files and splits. Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d --- M be/src/common/global-flags.cc M be/src/exec/base-sequence-scanner.cc M be/src/exec/base-sequence-scanner.h M be/src/exec/filter-context.cc M be/src/exec/filter-context.h M be/src/exec/hdfs-parquet-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scanner.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M be/src/runtime/runtime-filter-ir.cc M be/src/runtime/runtime-filter.h M be/src/runtime/runtime-filter.inline.h M be/src/util/bloom-filter-ir.cc M be/src/util/bloom-filter.cc M be/src/util/bloom-filter.h M common/thrift/ImpalaInternalService.thrift A tests/custom_cluster/test_always_false_filter.py 21 files changed, 228 insertions(+), 198 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8170/5 -- To view, visit http://gerrit.cloudera.org:8080/8170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d Gerrit-Change-Number: 8170 Gerrit-PatchSet: 5 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6535 ) Change subject: IMPALA-2550: Switch to per-query exec rpc .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/6535/15/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/6535/15/be/src/runtime/coordinator-backend-state.cc@363 PS15, Line 363: local_params.__set_bloom_filter(rpc_params->bloom_filter); > Cleanup is definitely welcome. Came across this while looking at Tianyi's patch. It looks like this patch actually removed the parallelisation. Before this patch the filter distribution was offloaded to exec_env_->rpc_pool(). After this change the filter distribution is done serially on the RPC thread that processes the UpdateFilter() message. I'm not sure what the consequences of this are, but we should keep an eye on this to see if it caused any regressions in Impala 2.9. Unfortunately it looks like the change was snuck into the larger changes in this patch - it looks like it wasn't mentioned in the commit message or noticed by reviewers. Before the code was: exec_env_->rpc_pool()->Offer(bind(DistributeFilters, rpc_params, fragment_inst->impalad_address(), fragment_inst->fragment_instance_id())); -- To view, visit http://gerrit.cloudera.org:8080/6535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd Gerrit-Change-Number: 6535 Gerrit-PatchSet: 15 Gerrit-Owner: Marcel Kornacker Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 13 Oct 2017 18:56:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5429: Multi threaded block metadata loading
Vuk Ercegovac has posted comments on this change. (
http://gerrit.cloudera.org:8080/8235 )
Change subject: IMPALA-5429: Multi threaded block metadata loading
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8235/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/8235/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@368
PS5, Line 368: for (HdfsPartition partition: partitions)
partition.setFileDescriptors(
> Am I misreading this or does each partition get set to the same list of new
Just clarified this so I'll post my misunderstanding here. The comment states
that "partitions in 'partitions' that correspond to the path 'partDir'".
"correspond" is open to interpretation so I understood this as:
/.../partDir/partition1_dir, partition2_dir, etc.
The intended interpretation is that multiple partitions map (or refer) to the
same fs path ('pathDir') (usecase: "latest" is an alias for the most recently
populated partition).
--
To view, visit http://gerrit.cloudera.org:8080/8235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I07eaa7151dfc4d56da8db8c2654bd65d8f808481
Gerrit-Change-Number: 8235
Gerrit-PatchSet: 5
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Mostafa Mokhtar
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Fri, 13 Oct 2017 18:36:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/7954 ) Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala .. Patch Set 15: Csaba, are those failing tests specifically require a timestamp that issues such a warning? If not, I suggest we just change the timestamps of those tests in a way that preserves test coverage and avoids the warnings problem. -- To view, visit http://gerrit.cloudera.org:8080/7954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff Gerrit-Change-Number: 7954 Gerrit-PatchSet: 15 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Comment-Date: Fri, 13 Oct 2017 18:16:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1335/ -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 13 Oct 2017 17:13:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: Code-Review+2 Thanks for fixing this. I fixed it in the same way but couldn't get a local build+test to run for unrelated reasons. -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 13 Oct 2017 17:09:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/8262 )
Change subject: IMPALA-6045: Make build scripts more friendly to Ubuntu 16.04
..
Patch Set 3:
(2 comments)
Sorry to get to this a bit late.
I find the distinctions between the (now 3) bootstrap_* scripts to be really
hard to grok. Since we're multiplying scripts in this commit, it seems like we
should clarify those distinctions.
I very much like separating bootstrap_system from bootstrap_development, but I
think it doesn't make sense to separate it into two scripts. Rather, I think
this would be clearer to use if it looked like:
bootstrap_development [install-packages] [checkout-impala] [configure]
[build] [loadtestdata] [runtests]
(Or something like it; point being to have on entry point with a few variants.)
The default actions would be whatever bootstrap_development does today.
There's a mild prototype at
https://github.com/philz/incubator-impala/commit/e0e7ad3ce07335709987b1b4088ac66ef312e7af
if you're interested. This came up for me while trying to time the different
parts of the bootstrap script for some Docker work I was doing. In it, I just
broke the existing script into ~5 bash functions.
http://gerrit.cloudera.org:8080/#/c/8262/3/bin/bootstrap_build.sh
File bin/bootstrap_build.sh:
http://gerrit.cloudera.org:8080/#/c/8262/3/bin/bootstrap_build.sh@35
PS3, Line 35: python-dev python-setuptools libffi-dev libkrb5-dev
Consider just adding openjdk-${JDK_VERSION}-jdk (and friends) here? I don't
know that one apt-get command is faster than two, but it's nice to have them
all together.
http://gerrit.cloudera.org:8080/#/c/8262/3/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:
http://gerrit.cloudera.org:8080/#/c/8262/3/bin/bootstrap_system.sh@20
PS3, Line 20: # This script bootstraps a system for Impala development from
almost nothing; it is known
This is very heavily replicated with bootstrap_development; highlighting the
differences may make sense.
--
To view, visit http://gerrit.cloudera.org:8080/8262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8196a2a87bce5893a349a1b290c3f3d04fd80317
Gerrit-Change-Number: 8262
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Michael Brown
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Zoram Thanga
Gerrit-Comment-Date: Fri, 13 Oct 2017 16:54:34 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Fri, 13 Oct 2017 16:44:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8272 ) Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. Patch Set 1: If you would like me to perform a file TARGET_FILESYSTEM="local" build+test, let me know, but it's pretty clear what the problem was and that the skipping works now. -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Comment-Date: Fri, 13 Oct 2017 16:30:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem
Michael Brown has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8272 Change subject: IMPALA-6049: breakpad tests: skip all tests with local filesystem .. IMPALA-6049: breakpad tests: skip all tests with local filesystem The breakpad tests were recently refactored to support inclusion of one of them as a core test. In this refactor, we neglected to ensure setup_class() called its parent. This means the skipping called in said parent doesn't occur, and the test is executed in an unsupported environment (local filesystem). This patch fixes that by ensuring we call the parent setup_class() via super(). Testing: $ TARGET_FILESYSTEM="local" impala-py.test tests/custom_cluster/test_breakpad.py \ -k test_abort_writes_minidump tests/custom_cluster/test_breakpad.py::TestBreakpadCore::test_abort_writes_minidump SKIPPED Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 --- M tests/custom_cluster/test_breakpad.py 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8272/1 -- To view, visit http://gerrit.cloudera.org:8080/8272 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95 Gerrit-Change-Number: 8272 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Fri, 13 Oct 2017 16:09:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/7954 ) Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/7954/9/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/7954/9/be/src/exprs/timestamp-functions-ir.cc@139 PS9, Line 139: } > Literal::Literal(ColumnType type, double v) also uses TimestampValue::FromS I have found the cause of this issue, see https://issues.apache.org/jira/browse/IMPALA-5664?focusedCommentId=16203482&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16203482 -- To view, visit http://gerrit.cloudera.org:8080/7954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff Gerrit-Change-Number: 7954 Gerrit-PatchSet: 9 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Comment-Date: Fri, 13 Oct 2017 13:05:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger, anujphadke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8038 to look at the new patch set (#7). Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. IMPALA-5736: Add impala-shell argument to set default query options Query options can be set from command line and impala rc as key=value pairs. Examples: command line: impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200 .impalarc: [impala.query_options] EXPLAIN_LEVEL=2 MT_DOP=2 The options set in command line will update the ones in impalarc one by one, so the result of the example above will be: EXPLAIN_LEVEL=2 MT_DOP=1 MAX_ERRORS=200 Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 --- M shell/impala_shell.py M shell/option_parser.py A tests/shell/impalarc_with_query_options M tests/shell/test_shell_interactive.py 4 files changed, 101 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/7 -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: anujphadke
