[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 04:52:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3302/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 04:52:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 04:49:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 ) Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1021/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 11 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 11 Oct 2018 03:14:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11651 ) Change subject: IMPALA-7693: stress test: fix Query().name .. Patch Set 2: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3301/ -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Brown Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 11 Oct 2018 02:49:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Michal Ostrowski has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 ) Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@40 PS9, Line 40: /// Single NaN values to ensure all NaN values can be assigned one bit pattern : /// that will always compare and hash t > Done Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@107 PS9, Line 107: val/type c > I don't see the point of keeping this ambiguity unless you can come up with Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@112 PS9, Line 112: CanonicalNaNVa > Ack Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@112 PS9, Line 112: ue(const ColumnTyp > Done Done -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 11 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 11 Oct 2018 02:40:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11535 to look at the new patch set (#11). Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. IMPALA-6661 Make NaN values equal for grouping purposes. Similar to the treatment of NULLs, we want to consider NaN values as equal when grouping. - When detecting a NaN in a set of row values, the NaN value must be converted to a canonical value - so that all NaN values have the same bit-pattern for hashing purposes. - When doing equality evaluation, floating point types must have additional logic to consider NaN values as equal. - Existing logic for handling NULLs in this way is appropriate for triggering this behavior for NaN values. - Relabel "force null equality" as "inclusive equality" to expand the scope of the concept to a more generic form that includes NaN. Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-anyval.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/hash-table.inline.h M be/src/runtime/raw-value.cc M be/src/runtime/raw-value.h M be/src/runtime/raw-value.inline.h M testdata/workloads/functional-query/queries/QueryTest/exprs.test 9 files changed, 165 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/11535/11 -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 11 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions()
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11641 ) Change subject: IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11641/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11641/1//COMMIT_MSG@11 PS1, Line 11: without locking > Doesn't the caller of the bulkAlterPartitions() hold the table lock? For ex I think the table is locked but bulkAlterPartitions is fetching the table again from table cache, which might be a different table object. -- To view, visit http://gerrit.cloudera.org:8080/11641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ec120f9df64d6e7e7d4978b5e190376721a6897 Gerrit-Change-Number: 11641 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 02:35:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1020/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 02:26:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 ) Change subject: IMPALA-7555: Set socket timeout in impala-shell .. Patch Set 7: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3300/ -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 7 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 11 Oct 2018 01:59:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/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/11649/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2892 PS6, Line 2892: his method : // is used as > The idea of this method is to basically do replicate what Sentry refresh do I updated the comment to make it clearer. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 01:56:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. IMPALA-7688: Fix spurious error messages when updating owner privileges Failure in updating owner privileges is not an issue since this could mean a Sentry refresh occurred while updating owner privileges and Sentry refresh itself will update all privileges including owner privileges. This patch changes the code to log the failure in updating owner privileges as WARN instead of ERROR. Testing: - Ran all FE tests - Ran all E2E authorization tests Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java 1 file changed, 31 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/11649/9 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/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/11649/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2892 PS6, Line 2892: whole method : // redundant. > didn't follow this part.. what do you mean by redundant? do you mean its "b The idea of this method is to basically do replicate what Sentry refresh does and yes, it's a best-effort so that users can start seeing the owner privileges without having to wait for a Sentry refresh. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 01:38:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions()
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/11641 )
Change subject: IMPALA-7670: Avoid getting the latest tables in
bulkAlterPartitions()
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11641/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11641/1//COMMIT_MSG@11
PS1, Line 11: without locking
Doesn't the caller of the bulkAlterPartitions() hold the table lock? For
example, in the stack mentioned in the jira, dropStats() takes a table lock
before it eventually calls bulkAlterPartitions(). With this lock, is it still
possible to have concurrent modifications of the same table?
private void dropStats(TDropStatsParams params, TDdlExecResponse resp)
throws ImpalaException {
Table table = getExistingTable(params.getTable_name().getDb_name(),
params.getTable_name().getTable_name());
Preconditions.checkNotNull(table);
if (!catalog_.tryLockTable(table)) { <=
throw new InternalException(String.format("Error dropping stats for table
%s " +
"due to lock contention", table.getFullName()));
}
--
To view, visit http://gerrit.cloudera.org:8080/11641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ec120f9df64d6e7e7d4978b5e190376721a6897
Gerrit-Change-Number: 11641
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 11 Oct 2018 01:15:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/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/11649/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2892 PS6, Line 2892: whole method : // redundant. didn't follow this part.. what do you mean by redundant? do you mean its "best-effort" here so if an exception happens, that's ok? -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 11 Oct 2018 01:05:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10855 )
Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use
KRPC
..
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h@146
PS16, Line 146: service_name
admittedly it's clever that you were able to hack the username in order to get
separate connections, but I'm finding it somewhat strange to follow when
looking at the code.
Here, it's not really clear why you need to pass the service name if you're
already passing the service class type -- the service name is already available
there by calling P::static_service_name(), so the parameter seems redundant.
Given that, as more of these services get converted over to KRPC, we probably
just want a 'data plane' and 'control plane', maybe it makes more sense to add
a new enum type like 'CommunicationPlane' with options kDataPlane and
kControlPlane? Internally, you could still implement this by hacking the
username for now, but at least it makes it clearer what the purpose of this
parameter is.
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc@506
PS16, Line 506: {
why is this extra indentation block here? doesn't seem like there is any RAII
or lock acquisition that needs this scope
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h@107
PS16, Line 107: const
nit: const non-reference parameters dont make much sense
http://gerrit.cloudera.org:8080/#/c/10855/12/common/protobuf/common.proto
File common/protobuf/common.proto:
http://gerrit.cloudera.org:8080/#/c/10855/12/common/protobuf/common.proto@33
PS12, Line 33: fixed
> Agreed that they don't tend to be small. Does using fixed64 make the encodi
yea, not that it really matters for this use case, but figured it's more
instructive
--
To view, visit http://gerrit.cloudera.org:8080/10855
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe
Gerrit-Change-Number: 10855
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michal Ostrowski
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 10 Oct 2018 23:18:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11650 )
Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h@260
PS1, Line 260: uint8_t* ALWAYS_INLINE Allocate(int64_t size, int alignment)
noexcept {
> After a first pass, it looks like this isn't our only mis-use of MemPools (
Good to hear that DFAKE_SCOPED_LOCK() is catching the problem.
How many other different instances are we talking about here ? My preference
would be to fix all of them in this patch as there is a workaround for the bug
in the min/max filter. Of course, if fixing some of those new cases requires a
lot of refactoring or if there are too many places which suffer from this
problem (which would be surprising), we can consider deferring the adding of
DFAKE_SCOPED_LOCK() here.
--
To view, visit http://gerrit.cloudera.org:8080/11650
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 10 Oct 2018 23:12:04 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11626 ) Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1019/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c Gerrit-Change-Number: 11626 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Wed, 10 Oct 2018 23:09:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. IMPALA-7682: Make AuthorizationPolicy thread-safe This patch makes AuthorizationPolicy thread-safe by making AuthorizationPolicy::listPrivileges(Set groups, Set users, ActiveRoleSet roleSet) synchronized. Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Reviewed-on: http://gerrit.cloudera.org:8080/11644 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 23:06:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/11650 )
Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h@260
PS1, Line 260: uint8_t* ALWAYS_INLINE Allocate(int64_t size, int alignment)
noexcept {
> May benefit from DFAKE_SCOPED_LOCK(). See https://gerrit.cloudera.org/#/c/1
After a first pass, it looks like this isn't our only mis-use of MemPools (i.e.
we're hitting the DFAKE_SCOPED_LOCK even with this patch)
I'll see about fixing that, but we may also want to consider getting this in as
is and doing the fake lock as a followup task.
--
To view, visit http://gerrit.cloudera.org:8080/11650
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 10 Oct 2018 23:02:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 ) Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243 PS2, Line 243: backend_exec_state_ = cur_state == BackendExecState::PREPARING ? : BackendExecState::EXECUTING : BackendExecState::FINISHED; > The state transition logic is determined by 'overall_status_'. So, making Sorry, to be clearer - I was suggesting leaving the logic around 'overall_state_' here and just changing the contents of this 'else' branch. My thinking was that for a particular call site of UpdateBackendExecState() we would know what state we expect to transition to if overall_state_ is OK, eg. the call on line 518 would just take EXECUTING as a param, which would help ensure we don't accidentally call UpdateBackendExecState() twice for the PREPARE->EXECUTING transition. Looking more closely, though, I see that this doesn't make sense for the call at line 510. I think its fine as is, no need to significantly rewrite this. Maybe just add a DCHECK at line 520 that the state is EXECUTING. -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 22:51:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11630 ) Change subject: IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support. .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1018/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5120b071760e7655e78902dce8483f8f54de445d Gerrit-Change-Number: 11630 Gerrit-PatchSet: 4 Gerrit-Owner: mackror...@apache.org Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: mackror...@apache.org Gerrit-Comment-Date: Wed, 10 Oct 2018 22:46:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11626 to look at the new patch set (#2). Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator .. IMPALA-7677: Fix DCHECK failure in GroupingAggregator After inserting all of its input into its Aggregators, StreamingAggregationNode performs some cleanup, such as calling InputDone() on each Aggregator. Previously, StreamingAggregationNode only checked that all of the child's batches had been fetched before doing this cleanup, which causes problems if the final child batch isn't processed fully in a single GetNext() call. In this case, multiple calls to InputDone() lead to a DCHECK failure. The solution is to only perform the cleanup once the final child batch has been fully processed. Testing: - Added an e2e test with a query that hits this condition. Change-Id: I851007a60472d0e53081c076c863c866c516677c --- M be/src/exec/streaming-aggregation-node.cc M testdata/workloads/functional-query/queries/QueryTest/multiple-distinct-aggs.test 2 files changed, 16 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/11626/2 -- To view, visit http://gerrit.cloudera.org:8080/11626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c Gerrit-Change-Number: 11626 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 ) Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. Patch Set 16: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1016/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Oct 2018 22:37:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 ) Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1017/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 22:32:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11651 ) Change subject: IMPALA-7693: stress test: fix Query().name .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3301/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Brown Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 22:24:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11651 ) Change subject: IMPALA-7693: stress test: fix Query().name .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Brown Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 22:24:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11651 ) Change subject: IMPALA-7693: stress test: fix Query().name .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1015/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 22:14:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 ) Change subject: IMPALA-7555: Set socket timeout in impala-shell .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 7 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 22:05:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 ) Change subject: IMPALA-7555: Set socket timeout in impala-shell .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3300/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 7 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 22:05:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 ) Change subject: IMPALA-7555: Set socket timeout in impala-shell .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 6 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 22:05:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support.
Hello Philip Zeyliger, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11630 to look at the new patch set (#4). Change subject: IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support. .. IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support. HADOOP-15407 adds a new FileSystem implementation called "ABFS" for the ADLS Gen2 service. It's in the hadoop-azure module as a replacement for WASB. Filesystem semantics should be the same, so skipped tests and other behavior changes have simply mirrored what is done for ADLS Gen1 by default. URI schemes are configured separately, however, so new functions are needed. This also allows behavior to be tweaked independently should any quirks be found in subsequent performance testing. Was not able to get a full test run completed due to snags in the data loading on non-HDFS storage. Consequently, abfs_util.py has not yet been through a full test run either. Patch was tested against TPC-DS with no apparent problems. Change-Id: I5120b071760e7655e78902dce8483f8f54de445d --- M be/src/exec/hdfs-table-sink.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h M be/src/runtime/io/scan-range.cc M be/src/util/hdfs-util.cc M be/src/util/hdfs-util.h M bin/impala-config.sh M fe/pom.xml M fe/src/main/java/org/apache/impala/analysis/LoadDataStmt.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl M tests/common/impala_test_suite.py M tests/common/skip.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_hdfs_fd_caching.py M tests/custom_cluster/test_insert_behaviour.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_parquet_max_page_header.py M tests/custom_cluster/test_permanent_udfs.py M tests/data_errors/test_data_errors.py M tests/failure/test_failpoints.py M tests/metadata/test_compute_stats.py M tests/metadata/test_hdfs_encryption.py M tests/metadata/test_hdfs_permissions.py M tests/metadata/test_hms_integration.py M tests/metadata/test_metadata_query_statements.py M tests/metadata/test_partition_metadata.py M tests/metadata/test_refresh_partition.py M tests/metadata/test_views_compatibility.py M tests/query_test/test_compressed_formats.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_insert_behaviour.py M tests/query_test/test_insert_parquet.py M tests/query_test/test_join_queries.py M tests/query_test/test_nested_types.py M tests/query_test/test_observability.py M tests/query_test/test_partitioning.py M tests/query_test/test_scanners.py M tests/stress/test_ddl_stress.py A tests/util/abfs_util.py M tests/util/filesystem_utils.py 44 files changed, 320 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/11630/4 -- To view, visit http://gerrit.cloudera.org:8080/11630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5120b071760e7655e78902dce8483f8f54de445d Gerrit-Change-Number: 11630 Gerrit-PatchSet: 4 Gerrit-Owner: mackror...@apache.org Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: mackror...@apache.org
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/11410 ) Change subject: IMPALA-6249: Expose several build flags via web UI .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py@40 PS9, Line 40: 'webserver' > what do you think about doing something to prevent these sorts of errors in I think your approach makes a lot of sense. I'd also be comfortable with just having a blacklist. It looks like we already expected that adding a directory would include it in the execution, and if people get bitten by it, they can add it here. Alternatively we could add some magic DONT-TEST-THIS file in each folder that shouldn't get executed. I don't feel strongly and am good with either approach. -- To view, visit http://gerrit.cloudera.org:8080/11410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 22:02:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11615 )
Change subject: IMPALA-4063: Merge report of query fragment instances per
executor
..
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@185
PS2, Line 185: status reports periodically
: /// to
> nit: status reports periodically to the
Done
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@428
PS2, Line 428: Should only be called when
: /// the current state is a non-terminal state. T
> A little confusing - its required to currently be in a non-terminal state w
Done
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@227
PS2, Line 227: {
> any reason to do this here, when its only used within the lock below?
Done
http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243
PS2, Line 243: backend_exec_state_ = cur_state == BackendExecState::PREPARING ?
: BackendExecState::EXECUTING :
BackendExecState::FINISHED;
> This feels a little bit weird - would it work to make this more explicit by
The state transition logic is determined by 'overall_status_'. So, making
UpdateBackendExecState() take a param of the next state means moving the logic
encoded in the if-stmt above elsewhere and UpdateBackendExecState() simply
becomes a setter of backend_exec_state_ with some DCHECKs(). I can see how it
may be slightly clearer for the transition from PREPARING state to the next
legal state but it may make the code slightly more hairy at the multiple call
sites.
Will it be clearer if we update the DCHECK at line 231 to make it more explicit
which valid states we could be at so it's easier to reason about the state
transition logic ?
http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:
http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto@150
PS2, Line 150: // The fragment instance id of the first failed fragment
instance.
> Maybe worth making it more explicit that the reason we chose the 'first' he
It's not set for "general" error. Comments clarified to update its relationship
with "overall_status".
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 10 Oct 2018 21:57:22 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, Michal Ostrowski, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10855 to look at the new patch set (#16). Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC This change converts ReportExecStatus() RPC from thrift based RPC to KRPC. This is done in part of the preparation for fixing IMPALA-2990 as we can take advantage of TCP connection multiplexing in KRPC to avoid overwhelming the coordinator with too many connections by reducing the number of TCP connection to one for each executor. This patch also introduces a new service pool for all query execution control related RPCs in the future so that control commands from coordinators aren't blocked by long-running DataStream services' RPCs. To avoid unnecessary delays due to sharing the network connections between DataStream service and Control service, this change added the service name as part of the user credentials for the ConnectionId so each service will use a separate connection. The majority of this patch is mechanical conversion of some Thrift structures used in ReportExecStatus() RPC to Protobuf. Note that the runtime profile is still retained as a Thrift structure as Impala clients will still fetch query profiles using Thrift RPCs. This also avoids duplicating the serialization implementation in both Thrift and Protobuf for the runtime profile. The Thrift runtime profiles are serialized and sent as a sidecar in ReportExecStatus() RPC. This patch also fixes IMPALA-7241 which may lead to duplicated dml stats being applied. The fix is by adding a monotonically increasing version number for fragment instances' reports. The coordinator will ignore any report smaller than or equal to the version in the last report. Testing done: 1. Exhaustive build. 2. Added some targeted test cases for profile serialization failure and RPC retries/timeout. Change-Id: I7638583b433dcac066b87198e448743d90415ebe --- M be/src/benchmarks/expr-benchmark.cc M be/src/catalog/catalog-util.cc M be/src/common/global-flags.cc M be/src/exec/data-sink.cc M be/src/exec/data-sink.h M be/src/exec/hbase-table-sink.cc M be/src/exec/hdfs-parquet-table-writer.cc M be/src/exec/hdfs-parquet-table-writer.h M be/src/exec/hdfs-table-sink.cc M be/src/exec/hdfs-table-writer.cc M be/src/exec/hdfs-table-writer.h M be/src/rpc/CMakeLists.txt M be/src/rpc/jni-thrift-util.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.h M be/src/rpc/rpc-mgr.inline.h M be/src/rpc/thrift-util-test.cc M be/src/rpc/thrift-util.h M be/src/runtime/backend-client.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/dml-exec-state.cc M be/src/runtime/dml-exec-state.h M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/krpc-data-stream-sender.cc M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/test-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/scheduler-test-util.cc M be/src/service/CMakeLists.txt M be/src/service/client-request-state.cc M be/src/service/client-request-state.h A be/src/service/control-service.cc A be/src/service/control-service.h M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h 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/in-process-servers.cc M be/src/util/container-util.h A be/src/util/error-util-internal.h M be/src/util/error-util-test.cc M be/src/util/error-util.cc M be/src/util/error-util.h M be/src/util/runtime-profile.cc M be/src/util/uid-util.h M common/protobuf/CMakeLists.txt M common/protobuf/common.proto A common/protobuf/control_service.proto M common/protobuf/data_stream_service.proto M common/protobuf/row_batch.proto M common/protobuf/rpc_test.proto M common/thrift/ImpalaInternalService.thrift M tests/custom_cluster/test_rpc_timeout.py 66 files changed, 1,287 insertions(+), 787 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/16 -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Ho Gerrit-Reviewer:
[Impala-ASF-CR] IMPALA-7677: Fix DCHECK failure in GroupingAggregator
Thomas Marshall has removed Philip Zeyliger from this change. ( http://gerrit.cloudera.org:8080/11626 ) Change subject: IMPALA-7677: Fix DCHECK failure in GroupingAggregator .. Removed reviewer Philip Zeyliger. -- To view, visit http://gerrit.cloudera.org:8080/11626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I851007a60472d0e53081c076c863c866c516677c Gerrit-Change-Number: 11626 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/11651 ) Change subject: IMPALA-7693: stress test: fix Query().name .. Patch Set 1: Code-Review+2 Looks straightforward enough. Thanks for the quick fix. -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 21:44:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7693: stress test: fix Query().name
Michael Brown has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11651 Change subject: IMPALA-7693: stress test: fix Query().name .. IMPALA-7693: stress test: fix Query().name In the refactor as part of IMPALA-7460, loading of TPC queries no longer returned query names (i.e., Q37). The name's presence doesn't change the behavior of the stress test, but it does lead to nicer debuggable and reable things, like log messages, profiles, result hashes, and runtime info. - Change load_tpc_queries() to return a dictionary, not a list. - Set the .name attribute. - Enhance the unit test to at least ensure load_tpc_queries() does not regress again. Testing, in addition to passing test above: - Ran stress test and performed binary search. Made sure query names were present in logging, runtime info, result hashes, and profiles. Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 --- M tests/infra/test_stress_infra.py M tests/stress/concurrent_select.py M tests/util/test_file_parser.py 3 files changed, 10 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/11651/1 -- To view, visit http://gerrit.cloudera.org:8080/11651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie8c40beababf4c122dc8fed6c0544ee37871b9b2 Gerrit-Change-Number: 11651 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Brown
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 ) Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@185 PS2, Line 185: status report periodically : /// the nit: status reports periodically to the http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.h@428 PS2, Line 428: A state transition happens : /// if the current state is a non-terminal state A little confusing - its required to currently be in a non-terminal state when this is called. http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@227 PS2, Line 227: BackendExecState old_state = backend_exec_state_; any reason to do this here, when its only used within the lock below? http://gerrit.cloudera.org:8080/#/c/11615/2/be/src/runtime/query-state.cc@243 PS2, Line 243: backend_exec_state_ = old_state == BackendExecState::PREPARING ? : BackendExecState::EXECUTING : BackendExecState::FINISHED; This feels a little bit weird - would it work to make this more explicit by having UpdateBackendExecState take a param of the state to transition to, and then DCHECK-ing that the param is the next state in the lifecycle? http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto@150 PS2, Line 150: // The fragment instance id of the first failed fragment instance. Maybe worth making it more explicit that the reason we chose the 'first' here is that it corresponds to the value of 'overall_status' Also, what's the expected value here if 'overall_status' is for a non-fragment-specific "general" error? -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 21:04:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11540 ) Change subject: IMPALA-7555: Set socket timeout in impala-shell .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1014/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 6 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 20:54:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 ) Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/10855/15/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/10855/15/common/protobuf/control_service.proto@113 PS15, Line 113: int32 int64 -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Oct 2018 20:48:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11410 ) Change subject: IMPALA-6249: Expose several build flags via web UI .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py@40 PS9, Line 40: 'webserver' what do you think about doing something to prevent these sorts of errors in the future, eg: https://github.com/twmarshall/impala/commit/f5b6937a01d6a0b495a1ddb49e268f9bc08db689 (I had noticed this problem, filed IMPALA-7691, and started to fix it before realizing you already had a fix out) -- To view, visit http://gerrit.cloudera.org:8080/11410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 20:44:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Anuj Phadke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11540 )
Change subject: IMPALA-7555: Set socket timeout in impala-shell
..
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/impala_client.py
File shell/impala_client.py:
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/impala_client.py@81
PS5, Line 81: self.fetch_batch_size = 1024
> nit: move below use_ldap to follow order or arguments
Done
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/impala_client.py@255
PS5, Line 255: if self.client_connect_timeout_ms > 0:
> This comment can be moved to above line 264.
Done
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/impala_client.py@258
PS5, Line 258: protocol = TBinaryProtocol.TBinaryProtocol(self.transport)
: self.imp_service = Impa
> Given sock is just returned from self._get_socket_and_transport(), I don't
Done
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/option_parser.py@224
PS5, Line 224: parser.add_option("-t", "--client_connect_timeout_ms",
> nit: no newline, follow surrounding code
Done
http://gerrit.cloudera.org:8080/#/c/11540/5/shell/option_parser.py@227
PS5, Line 227: " timeout.")
> Set to 0 to disable any timeout.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813
Gerrit-Change-Number: 11540
Gerrit-PatchSet: 6
Gerrit-Owner: Anuj Phadke
Gerrit-Reviewer: Anuj Phadke
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 10 Oct 2018 20:27:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7555: Set socket timeout in impala-shell
Hello Michael Ho, Thomas Marshall, Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11540 to look at the new patch set (#6). Change subject: IMPALA-7555: Set socket timeout in impala-shell .. IMPALA-7555: Set socket timeout in impala-shell impala-shell does not set any socket timeout while connecting to the impala server. This change sets a timeout on the socket before connecting and unsets it back after successfully connecting. The default timeout on this socket is 1 min. Usage: impala-shell --client_connect_timeout= Testing: 1. Added a test where I create a random listening socket. impala-shell connects to this socket and times out after the default timeout value. 2. Created a kerberized impala cluster with ssl enabled and connected to the impalad using an openssl client (block the beeswax server thread to accept new connection) - E.g. - openssl s_client -connect :21000 Used impala-shell to connect to the same impalad later. impala-shell timed out after the default of 1 min.I verified it manually. Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 --- M shell/impala_client.py M shell/impala_shell.py M shell/impala_shell_config_defaults.py M shell/option_parser.py M tests/shell/test_shell_commandline.py 5 files changed, 40 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/11540/6 -- To view, visit http://gerrit.cloudera.org:8080/11540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I130fc47f7a83f591918d6842634b4e5787d00813 Gerrit-Change-Number: 11540 Gerrit-PatchSet: 6 Gerrit-Owner: Anuj Phadke Gerrit-Reviewer: Anuj Phadke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11650 ) Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1012/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 Gerrit-Change-Number: 11650 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 10 Oct 2018 19:45:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11650 )
Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter
..
Patch Set 1:
(3 comments)
The fix makes sense to me. Can you please try the DFAKE_SCOPED_LOCK()
suggestion and use a query with multiple joins inside the same fragment to see
if you can trigger the crash ?
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h
File be/src/runtime/mem-pool.h:
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/runtime/mem-pool.h@260
PS1, Line 260: uint8_t* ALWAYS_INLINE Allocate(int64_t size, int alignment)
noexcept {
May benefit from DFAKE_SCOPED_LOCK(). See
https://gerrit.cloudera.org/#/c/10855/15/be/src/runtime/fragment-instance-state.cc@396
Same for other functions.
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/util/min-max-filter.h
File be/src/util/min-max-filter.h:
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/util/min-max-filter.h@80
PS1, Line 80: /// Returns a new MinMaxFilter with the given type, allocated
from 'pool'.
Please add a comment for 'mem_tracker'
http://gerrit.cloudera.org:8080/#/c/11650/1/be/src/util/min-max-filter.h@186
PS1, Line 186: MemPool mem_pool_;
Please add a comment for it.
--
To view, visit http://gerrit.cloudera.org:8080/11650
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29
Gerrit-Change-Number: 11650
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Wed, 10 Oct 2018 19:23:38 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3299/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 19:18:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 19:18:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions()
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11641 ) Change subject: IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11641/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11641/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3263 PS1, Line 3263: List hmsPartitions = Lists.newArrayList(); line too long (96 > 90) -- To view, visit http://gerrit.cloudera.org:8080/11641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ec120f9df64d6e7e7d4978b5e190376721a6897 Gerrit-Change-Number: 11641 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 19:17:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions()
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11641 Change subject: IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions() .. IMPALA-7670: Avoid getting the latest tables in bulkAlterPartitions() bulkAlterPartitions() needs to mark altered partitions as dirty. It currently gets the latest table using table names and mark partitions in them as dirty without locking, which could lead to concurrent modifications of a table. This patch changes it into marking the tables it operates on dirty. Change-Id: I0ec120f9df64d6e7e7d4978b5e190376721a6897 --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java 1 file changed, 21 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/41/11641/1 -- To view, visit http://gerrit.cloudera.org:8080/11641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0ec120f9df64d6e7e7d4978b5e190376721a6897 Gerrit-Change-Number: 11641 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11650 ) Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter .. Patch Set 1: Nice catch. I'd be interested in a way that we could DCHECK that all users of mempool only come from one thread, if that's in fact the API. -- To view, visit http://gerrit.cloudera.org:8080/11650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 Gerrit-Change-Number: 11650 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 10 Oct 2018 19:05:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7272: Fix crash in StringMinMaxFilter
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11650 Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter .. IMPALA-7272: Fix crash in StringMinMaxFilter StringMinMaxFilter uses a MemPool to allocate space for StringBuffers. Previously, the MemPool was created by RuntimeFilterBank and passed to each StringMinMaxFilter. In queries with multiple StringMinMaxFilters being generated by the same fragment instance, this leads to overlapping use of the MemPool by different threads, which is incorrect as MemPools are not thread-safe. The solution is to have each StringMinMaxFilter create its own MemPool. This patch also documents MemPool as not thread-safe. Testing: - I have been unable to repro the actual crash despite trying a large variety of different things. However, with additional logging added its clear that the MemPool is being used concurrently, which is incorrect. Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 --- M be/src/runtime/mem-pool.h M be/src/runtime/runtime-filter-bank.cc M be/src/runtime/runtime-filter-bank.h M be/src/util/min-max-filter.cc M be/src/util/min-max-filter.h 5 files changed, 25 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/11650/1 -- To view, visit http://gerrit.cloudera.org:8080/11650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 Gerrit-Change-Number: 11650 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-7678: Reapply "IMPALA-7660: Support ECDH ciphers for debug webserver"
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11625 ) Change subject: IMPALA-7678: Reapply "IMPALA-7660: Support ECDH ciphers for debug webserver" .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11625/1/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/11625/1/tests/common/impala_service.py@62 PS1, Line 62: context=context > One option is to use python for our non-SSL stuff and call out to wget or a I think I'll just switch to using the 'requests' library, which is in infra/python/deps/requirements.txt so it shouldn't suffer from these version issues. I've run into some additional problems that need to be worked through though. -- To view, visit http://gerrit.cloudera.org:8080/11625 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I679469ed7f27944f75004ec4b16d513e6ea6b544 Gerrit-Change-Number: 11625 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 18:37:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] [DOCS] Built-in Functions doc format Changes
Alex Rodoni has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11619 ) Change subject: [DOCS] Built-in Functions doc format Changes .. [DOCS] Built-in Functions doc format Changes - The function titles were changed to upper case. - The function titles no longer use . font appears smaller than the font. - Return type were changed to upper case data types. - Minor typos were fixed, such as extra commas and periods in titles. - The indexterm dita elememts were removed. Indexterm was incomplete and WIP. No plan to go ahead and implement it, so removed. Change-Id: I797532463da8d29fe5bc7543cfdfb5b2b82db197 Reviewed-on: http://gerrit.cloudera.org:8080/11619 Tested-by: Impala Public Jenkins Reviewed-by: Michael Brown --- M docs/shared/impala_common.xml M docs/topics/impala_aggregate_functions.xml M docs/topics/impala_analytic_functions.xml M docs/topics/impala_bit_functions.xml M docs/topics/impala_conditional_functions.xml M docs/topics/impala_conversion_functions.xml M docs/topics/impala_datetime_functions.xml M docs/topics/impala_functions.xml M docs/topics/impala_math_functions.xml M docs/topics/impala_misc_functions.xml M docs/topics/impala_string_functions.xml 11 files changed, 3,379 insertions(+), 2,721 deletions(-) Approvals: Impala Public Jenkins: Verified Michael Brown: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I797532463da8d29fe5bc7543cfdfb5b2b82db197 Gerrit-Change-Number: 11619 Gerrit-PatchSet: 4 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Michael Brown
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 ) Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/11615/2/common/protobuf/control_service.proto@113 PS2, Line 113: int32 int64. Will update in PS3. -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 18:21:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] [DOCS] Built-in Functions doc format Changes
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/11619 ) Change subject: [DOCS] Built-in Functions doc format Changes .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I797532463da8d29fe5bc7543cfdfb5b2b82db197 Gerrit-Change-Number: 11619 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Michael Brown Gerrit-Comment-Date: Wed, 10 Oct 2018 18:15:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7680: [DOCS] Minor edits to the command line config section
Alex Rodoni has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11636 ) Change subject: IMPALA-7680: [DOCS] Minor edits to the command line config section .. IMPALA-7680: [DOCS] Minor edits to the command line config section Change-Id: I3a3115feb2de1112ff0d2687a83a1307bd0791a5 Reviewed-on: http://gerrit.cloudera.org:8080/11636 Tested-by: Impala Public Jenkins Reviewed-by: Bikramjeet Vig --- M docs/topics/impala_admission.xml 1 file changed, 31 insertions(+), 27 deletions(-) Approvals: Impala Public Jenkins: Verified Bikramjeet Vig: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3a3115feb2de1112ff0d2687a83a1307bd0791a5 Gerrit-Change-Number: 11636 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] [DOCS] Copy edited the Cancelling a Query section
Alex Rodoni has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11637 ) Change subject: [DOCS] Copy edited the Cancelling a Query section .. [DOCS] Copy edited the Cancelling a Query section Change-Id: I7a06c94b397c872636d2ecf68cc6a2e193a3b047 Reviewed-on: http://gerrit.cloudera.org:8080/11637 Tested-by: Impala Public Jenkins Reviewed-by: Bikramjeet Vig --- M docs/topics/impala_timeouts.xml 1 file changed, 7 insertions(+), 9 deletions(-) Approvals: Impala Public Jenkins: Verified Bikramjeet Vig: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7a06c94b397c872636d2ecf68cc6a2e193a3b047 Gerrit-Change-Number: 11637 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 ) Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@107 PS9, Line 107: IsAmbiguous > The ambiguity is intentional so as to not define float/double-specific mech I don't see the point of keeping this ambiguity unless you can come up with reasonable examples which are applicable to other types. The general rule of operation is that we try not to generalize things too much until there are needs for it. -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 9 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 17:52:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support.
mackror...@apache.org has posted comments on this change. ( http://gerrit.cloudera.org:8080/11630 ) Change subject: IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support. .. Patch Set 3: > Quick comment: I think we are going to be skipping the same tests > for ADLS and ABFS. I would rather smash them together and reuse the > existing skipping condition we have for ADLS to cover both. That > should eliminate the need for a bunch of test changes. We can > always split it out later if we need to. There are already multiple cases where they diverge, actually: ADLS's Python client was apparently eventually consistent for some reason. That should not be the case with ABFS, however. So tests tagged with SkipIfADLS.eventually_consistent should not be skipped if it's ABFS. -- To view, visit http://gerrit.cloudera.org:8080/11630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5120b071760e7655e78902dce8483f8f54de445d Gerrit-Change-Number: 11630 Gerrit-PatchSet: 3 Gerrit-Owner: mackror...@apache.org Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: mackror...@apache.org Gerrit-Comment-Date: Wed, 10 Oct 2018 17:47:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support.
mackror...@apache.org has posted comments on this change. ( http://gerrit.cloudera.org:8080/11630 ) Change subject: IMPALA-7681. Add Azure Blob File System (ADLS Gen2) support. .. Patch Set 3: > (1 comment) No, thank you. Will include this tweak in my re-testing of all the other style changes and resubmit. -- To view, visit http://gerrit.cloudera.org:8080/11630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5120b071760e7655e78902dce8483f8f54de445d Gerrit-Change-Number: 11630 Gerrit-PatchSet: 3 Gerrit-Owner: mackror...@apache.org Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: mackror...@apache.org Gerrit-Comment-Date: Wed, 10 Oct 2018 17:45:57 + Gerrit-HasComments: No
[Impala-ASF-CR] [DOCS] Copy edited the Cancelling a Query section
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11637 ) Change subject: [DOCS] Copy edited the Cancelling a Query section .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a06c94b397c872636d2ecf68cc6a2e193a3b047 Gerrit-Change-Number: 11637 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 17:18:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7680: [DOCS] Minor edits to the command line config section
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11636 ) Change subject: IMPALA-7680: [DOCS] Minor edits to the command line config section .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a3115feb2de1112ff0d2687a83a1307bd0791a5 Gerrit-Change-Number: 11636 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 17:18:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7678: Reapply "IMPALA-7660: Support ECDH ciphers for debug webserver"
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11625 ) Change subject: IMPALA-7678: Reapply "IMPALA-7660: Support ECDH ciphers for debug webserver" .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11625/1/tests/common/impala_service.py File tests/common/impala_service.py: http://gerrit.cloudera.org:8080/#/c/11625/1/tests/common/impala_service.py@62 PS1, Line 62: context=context > https://docs.python.org/2/library/urllib2.html One option is to use python for our non-SSL stuff and call out to wget or another external utility for the SSL stuff. It adds complication, but I think it should work. We use wget in bootstrap_toolchain.py already. -- To view, visit http://gerrit.cloudera.org:8080/11625 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I679469ed7f27944f75004ec4b16d513e6ea6b544 Gerrit-Change-Number: 11625 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Pooja Nilangekar Gerrit-Comment-Date: Wed, 10 Oct 2018 17:14:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11624 ) Change subject: IMPALA-7554: Update custom cluster tests to have new logs for sentry .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1011/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516 Gerrit-Change-Number: 11624 Gerrit-PatchSet: 2 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 16:55:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1010/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 16:46:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11624 ) Change subject: IMPALA-7554: Update custom cluster tests to have new logs for sentry .. Patch Set 2: Code-Review+1 Csaba, can you also take another look at this? -- To view, visit http://gerrit.cloudera.org:8080/11624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516 Gerrit-Change-Number: 11624 Gerrit-PatchSet: 2 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Oct 2018 16:35:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7554: Update custom cluster tests to have new logs for sentry
Adam Holley has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11624 ) Change subject: IMPALA-7554: Update custom cluster tests to have new logs for sentry .. IMPALA-7554: Update custom cluster tests to have new logs for sentry This patch adds the ability to create a new log for each spawn of the sentry service. This will enable better trouble shooting for the custom cluster tests that restart the sentry service. Testing: - Ran all custom cluster tests. Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516 --- M testdata/bin/run-sentry-service.sh M tests/authorization/test_authorization.py M tests/authorization/test_owner_privileges.py M tests/common/custom_cluster_test_suite.py M tests/custom_cluster/test_redaction.py 5 files changed, 48 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11624/2 -- To view, visit http://gerrit.cloudera.org:8080/11624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6e538af7fd6e6ea21dc3f4442bdebf3b31558516 Gerrit-Change-Number: 11624 Gerrit-PatchSet: 2 Gerrit-Owner: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 16:20:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/5/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/11649/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2869 PS5, Line 2869: Preconditions.checkNotNull(ownerType); > Add precondition for filter not null. Done -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 16:14:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. IMPALA-7688: Fix spurious error messages when updating owner privileges Failure in updating owner privileges is not an issue since this could mean a Sentry refresh occurred while updating owner privileges and Sentry refresh itself will update all privileges including owner privileges. This patch changes the code to log the failure in updating owner privileges as WARN instead of ERROR. Testing: - Ran all FE tests - Ran all E2E authorization tests Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java 1 file changed, 27 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/11649/6 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1009/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 16:12:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11649 ) Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11649/5/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/11649/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2869 PS5, Line 2869: PrincipalPrivilege removedPrivilege = null; Add precondition for filter not null. -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 16:05:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 ) Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1008/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 10 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 15:45:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7688: Fix spurious error messages when updating owner privileges
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11649 Change subject: IMPALA-7688: Fix spurious error messages when updating owner privileges .. IMPALA-7688: Fix spurious error messages when updating owner privileges Failure in updating owner privileges is not an issue since this could mean a Sentry refresh occurred while updating owner privileges and Sentry refresh itself will update all privileges including owner privileges. This patch changes the code to log the failure in updating owner privileges as WARN instead of ERROR. Testing: - Ran all FE tests - Ran all E2E authorization tests Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java 1 file changed, 21 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/11649/5 -- To view, visit http://gerrit.cloudera.org:8080/11649 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4b46df01c5675ffed6528b7a73e68518664d8be0 Gerrit-Change-Number: 11649 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Michal Ostrowski has posted comments on this change. ( http://gerrit.cloudera.org:8080/11535 ) Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. Patch Set 10: (24 comments) http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@236 PS9, Line 236: ensure > ensures Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@239 PS9, Line 239: /// value of the value. (Currently this only has an impact for NaN > May want to clarify that this is a no-op for types other than FLOAT and DOU Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@240 PS9, Line 240: loat and double values > ConvertNanToCanonicalForm(); Intentionally not named with "NaN" because the interface as defined now does not depend on it. It just so happens that for non-NaN, non-double/float values it is a no-op. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.h@249 PS9, Line 249: clusive_equality" > Warrants a quick comment. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@476 PS9, Line 476: llvm::Value* > llvm::Value* . Same below. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@477 PS9, Line 477: > Seems unnecessary to initialize this value here as it's set in all paths wh Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@483 PS9, Line 483: is_n > "is_nan" is a more fitting name. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@487 PS9, Line 487: default: : ; > May as well remove these lines. Needed to avoid compiler errors/warnings. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@747 PS9, Line 747: llvm::Value* > llvm::Value* . Same below. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/codegen/codegen-anyval.cc@748 PS9, Line 748: llvm::Value* cmp_raw = builder_->CreateFCmpOEQ(local_val, val, "cmp_raw"); : if (!inclusive_equality) return cmp_raw; > nit: seems slightly easier to follow: I need to generate that comparison in either case, so if I followed you suggestion I'd need to follow that with: llvm::Value *cmp_raw = builder_->CreateFCmpOEQ(local_val, val, "cmp_raw"); Note that the current version uses "cmp_raw" in 2 places. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.h File be/src/exec/hash-table.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.h@464 PS9, Line 464: means "NULL==NULL" and "NaN==NaN". This > means "NULL == NULL" and "Nan == Nan" Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@198 PS9, Line 198: const ColumnType& > nit: const ColumnType& expr_type Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@257 PS9, Line 257: = R > huh ? Did you mean val ? Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@801 PS9, Line 801: > Don't we need to check for inclusive_equality flag here ? Also, why is the There is no "inclusive_equality" flag here. The restriction to probe-only is a continuation of the original logic for NULL's, which was based on that distinction. http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@1149 PS9, Line 1149: } : if (inclusive_equality) result.ConvertToCanonicalForm(); : > nit: one line Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/exec/hash-table.cc@1179 PS9, Line 1179: } > nit: extra blank line Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h File be/src/runtime/raw-value.h: http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@40 PS9, Line 40: /// Single NaN values to ensure all NaN values can be assigned one bit pattern : /// that will always compare and hash t > These deserve some comments on their purposes. Done http://gerrit.cloudera.org:8080/#/c/11535/9/be/src/runtime/raw-value.h@107 PS9, Line 107: there are > IsNan() seems to be a more appropriate name. The name and the description o The ambiguity is intentional so as to not define float/double-specific mechanisms into a generic interface. By keeping the ambiguity it allows for the interfaces to be used unconditionally, irrespective of the type of value being represented by a RawValue object. If these mechanisms were specifically defined for float/double, then the caller would have to consider whether or not it should use
[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.
Hello Michael Ho, Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11535 to look at the new patch set (#10). Change subject: IMPALA-6661 Make NaN values equal for grouping purposes. .. IMPALA-6661 Make NaN values equal for grouping purposes. Similar to the treatment of NULLs, we want to consider NaN values as equal when grouping. - When detecting a NaN in a set of row values, the NaN value must be converted to a canonical value - so that all NaN values have the same bit-pattern for hashing purposes. - When doing equality evaluation, floating point types must have additional logic to consider NaN values as equal. - Existing logic for handling NULLs in this way is appropriate for triggering this behavior for NaN values. - Relabel "force null equality" as "inclusive equality" to expand the scope of the concept to a more generic form that includes NaN. Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 --- M be/src/codegen/codegen-anyval.cc M be/src/codegen/codegen-anyval.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/hash-table.inline.h M be/src/runtime/raw-value.cc M be/src/runtime/raw-value.h M be/src/runtime/raw-value.inline.h M testdata/workloads/functional-query/queries/QueryTest/exprs.test 9 files changed, 168 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/11535/10 -- To view, visit http://gerrit.cloudera.org:8080/11535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086 Gerrit-Change-Number: 11535 Gerrit-PatchSet: 10 Gerrit-Owner: Michal Ostrowski Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3298/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 14:54:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 14:53:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 1: Code-Review+2 +1 from Csaba and + from Adam, giving it a +2. -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 14:53:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7682: Make AuthorizationPolicy thread-safe
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11644 ) Change subject: IMPALA-7682: Make AuthorizationPolicy thread-safe .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28c516c0ab3e48ab4284084afd7149ed1a3c6cb2 Gerrit-Change-Number: 11644 Gerrit-PatchSet: 1 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 10:44:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7676: DESCRIBE on table should require VIEW METADATA privilege
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11617 ) Change subject: IMPALA-7676: DESCRIBE on table should require VIEW_METADATA privilege .. IMPALA-7676: DESCRIBE on table should require VIEW_METADATA privilege IMPALA-6479 broke the DESCRIBE's privilege model by changing the privilege from VIEW_METADATA to ANY in order to support column-level privileges in DESCRIBE. This caused an issue where having non- VIEW_METADATA privilege, such as CREATE privilege on a particular database allows executing a DESCRIBE statement on all tables in the database. This behavior is also inconsistent with Hive's DESCRIBE and Impala's DESCRIBE DATABASE privilege models. Although there is not any security risk for this particular issue since having non- VIEW METADATA on a particular database always returns an empty result, fixing this issue will make the behavior consistent with Hive and also DESCRIBE DATABASE in Impala. This patch fixes the issue by changing the privilege requirement back from ANY to VIEW_METADATA. Testing: - Ran all FE tests Change-Id: I283e30ebff6d61e779a4cec8284cae0ccb90cc49 Reviewed-on: http://gerrit.cloudera.org:8080/11617 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java 7 files changed, 67 insertions(+), 42 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I283e30ebff6d61e779a4cec8284cae0ccb90cc49 Gerrit-Change-Number: 11617 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7676: DESCRIBE on table should require VIEW METADATA privilege
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11617 ) Change subject: IMPALA-7676: DESCRIBE on table should require VIEW_METADATA privilege .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11617 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I283e30ebff6d61e779a4cec8284cae0ccb90cc49 Gerrit-Change-Number: 11617 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 10 Oct 2018 09:03:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 ) Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1007/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Oct 2018 08:54:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 ) Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. Patch Set 15: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1006/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Oct 2018 08:46:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Hello Thomas Marshall, Todd Lipcon, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, Michal Ostrowski, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10855 to look at the new patch set (#15). Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC This change converts ReportExecStatus() RPC from thrift based RPC to KRPC. This is done in part of the preparation for fixing IMPALA-2990 as we can take advantage of TCP connection multiplexing in KRPC to avoid overwhelming the coordinator with too many connections by reducing the number of TCP connection to one for each executor. This patch also introduces a new service pool for all query execution control related RPCs in the future so that control commands from coordinators aren't blocked by long-running DataStream services' RPCs. To avoid unnecessary delays due to sharing the network connections between DataStream service and Control service, this change added the service name as part of the user credentials for the ConnectionId so each service will use a separate connection. The majority of this patch is mechanical conversion of some Thrift structures used in ReportExecStatus() RPC to Protobuf. Note that the runtime profile is still retained as a Thrift structure as Impala clients will still fetch query profiles using Thrift RPCs. This also avoids duplicating the serialization implementation in both Thrift and Protobuf for the runtime profile. The Thrift runtime profiles are serialized and sent as a sidecar in ReportExecStatus() RPC. This patch also fixes IMPALA-7241 which may lead to duplicated dml stats being applied. The fix is by adding a monotonically increasing version number for fragment instances' reports. The coordinator will ignore any report smaller than or equal to the version in the last report. Testing done: 1. Exhaustive build. 2. Added some targeted test cases for profile serialization failure and RPC retries/timeout. Change-Id: I7638583b433dcac066b87198e448743d90415ebe --- M be/src/benchmarks/expr-benchmark.cc M be/src/catalog/catalog-util.cc M be/src/common/global-flags.cc M be/src/exec/data-sink.cc M be/src/exec/data-sink.h M be/src/exec/hbase-table-sink.cc M be/src/exec/hdfs-parquet-table-writer.cc M be/src/exec/hdfs-parquet-table-writer.h M be/src/exec/hdfs-table-sink.cc M be/src/exec/hdfs-table-writer.cc M be/src/exec/hdfs-table-writer.h M be/src/rpc/CMakeLists.txt M be/src/rpc/jni-thrift-util.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.h M be/src/rpc/rpc-mgr.inline.h M be/src/rpc/thrift-util-test.cc M be/src/rpc/thrift-util.h M be/src/runtime/backend-client.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/dml-exec-state.cc M be/src/runtime/dml-exec-state.h M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/krpc-data-stream-sender.cc M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/runtime/test-env.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/scheduler-test-util.cc M be/src/service/CMakeLists.txt M be/src/service/client-request-state.cc M be/src/service/client-request-state.h A be/src/service/control-service.cc A be/src/service/control-service.h M be/src/service/data-stream-service.cc M be/src/service/data-stream-service.h 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/in-process-servers.cc M be/src/util/container-util.h A be/src/util/error-util-internal.h M be/src/util/error-util-test.cc M be/src/util/error-util.cc M be/src/util/error-util.h M be/src/util/runtime-profile.cc M be/src/util/uid-util.h M common/protobuf/CMakeLists.txt M common/protobuf/common.proto A common/protobuf/control_service.proto M common/protobuf/data_stream_service.proto M common/protobuf/row_batch.proto M common/protobuf/rpc_test.proto M common/thrift/ImpalaInternalService.thrift M tests/custom_cluster/test_rpc_timeout.py 66 files changed, 1,287 insertions(+), 787 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/10855/15 -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Ho Gerrit-Reviewer:
[Impala-ASF-CR] IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 ) Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC .. Patch Set 15: (6 comments) http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/runtime/coordinator-backend-state.cc@275 PS14, Line 275: // Hold the exec_summary's lock to avoid exposing it half-way through > Can we document the lock order in coordinator.h (which already references E Done http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/runtime/fragment-instance-state.h@177 PS14, Line 177: Monotonically > Typo: Monotonically Done http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/runtime/fragment-instance-state.h@179 PS14, Line 179: int64_t report_seq_no_ = 0; > I imagine you already thought about this and concluded that overflows were Done http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/testutil/in-process-servers.cc File be/src/testutil/in-process-servers.cc: http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/testutil/in-process-servers.cc@51 PS14, Line 51: FLAGS_krpc_port = > I guess we can't set this to 0 to automatically choose an ephemeral port? W Some tests were failing without this. I don't recall which one. Apparently, something is sourcing FLAGS_krpc_port so we have to set it to be consistent. http://gerrit.cloudera.org:8080/#/c/10855/14/be/src/testutil/in-process-servers.cc@51 PS14, Line 51: ; > Extra semicolon Done http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/10855/13/common/protobuf/control_service.proto@50 PS13, Line 50: } > Yeah, I think the rename makes sense to do, though not a big deal, and othe Done -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 15 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michal Ostrowski Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Oct 2018 08:11:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Hello Thomas Marshall, Tim Armstrong, Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11615 to look at the new patch set (#2). Change subject: IMPALA-4063: Merge report of query fragment instances per executor .. IMPALA-4063: Merge report of query fragment instances per executor Previously, each fragment instance executing on an executor will independently report its status to the coordinator periodically. This creates a huge amount of RPCs to the coordinator under highly concurrent workloads, causing lock contention in the coordinator's backend states as each fragment instance tries to them at the same time. In addition, due to the lack of coordination between query fragment instances, a query may end without collecting the profiles from all fragment instances when one of them hits an error before another fragment instance manages to finish calling Prepare(), leading to missing profiles for certain fragment instances. This change fixes the problem above by making a thread per QueryState (started by QueryExecMgr) to be responsible for periodically reporting the status and profiles of all fragment instances of a query running on a backend. As part of this refactoring, each query fragment instance will not report their errors individually. Instead, there is a cumulative status maintained per QueryState. It's set to the error status of the first fragment instance which hits an error or any general error (e.g. failure to start a thread) when starting fragment instances. With this change, the status reporting threads are also removed. Testing done: exhaustive tests This patch is based on a patch by Sailesh Mukil Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-exec-mgr.h M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/control-service.cc M common/protobuf/control_service.proto M common/thrift/RuntimeProfile.thrift M testdata/workloads/functional-query/queries/QueryTest/udf-non-deterministic.test M testdata/workloads/functional-query/queries/QueryTest/udf.test 17 files changed, 375 insertions(+), 426 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/11615/2 -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4063: Merge report of query fragment instances per executor
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11615 )
Change subject: IMPALA-4063: Merge report of query fragment instances per
executor
..
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.h@93
PS1, Line 93: /// Called periodically to get the current status of this
fragment instance.
> Since we're describing the usage pattern, might as well mention which threa
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.h@159
PS1, Line 159: bool final_report_sent_ = false;
> What's the thread safety story here?
Added some comments. This is exclusively accessed by the query state thread
only.
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.cc@97
PS1, Line 97: intance
> instance
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.cc@239
PS1, Line 239:
> nit: unnecessary blank line
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/fragment-instance-state.cc@243
PS1, Line 243: void
FragmentInstanceState::GetStatusReport(FragmentInstanceExecStatusPB*
instance_status,
> nit: could probably reduce vertical whitespace in this function.
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-exec-mgr.cc@128
PS1, Line 128: void QueryExecMgr::StartQueryHelper(QueryState* qs) {
> I feel like we should rename this function since it now runs until the quer
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.h@353
PS1, Line 353: /// 'overall_status_' will be set once this is unblocked and
so will 'failed_instance_id_'
> line too long (92 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.h@418
PS1, Line 418: bool IsStatusSet() const {
> Maybe something like "HasErrorStatus()". It's confusing that setting overal
Done
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.h@419
PS1, Line 419: return !overall_status_.ok() &&
!overall_status_.IsCancelled();
> This is called without holding status_lock_, right? I don't think IsCancell
Good point. I think the optimization doesn't really matter that much given it's
only exercised in the error path. It's okay to have a minor bit of lock
contention if multiple fragment instances hit errors at the same time.
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.cc@a553
PS1, Line 553:
:
The deletion of this line warrants some explanation:
In the old code, if there is any error returned from fis->Exec(), the final
report for "fis" would have been sent already so the coordinator will already
record the failure of "fis" before the cancellation is issued.
In the new code, the reporting is done periodically by the query state thread
so the final report may or may not be sent at this point after "fis" hit an
error above. Calling Cancel() here may actually cause the coordinator fragment
(if there is one) to prematurely set the backend status at the coordinator,
causing Coordinator::BackendState::ApplyExecStatusReport() to ignore subsequent
updates from "fis". As a result, the actual error status was masked.
The implicit contract in the code (even before this change) is that the final
report of the first fragment instance to hit an (non-cancellation) error must
have been sent before initiating the cancellation. Will add a comment here.
http://gerrit.cloudera.org:8080/#/c/11615/1/be/src/runtime/query-state.cc@236
PS1, Line 236: if (overall_status_.IsCancelled()) {
> Can we refine this with the internal/client-driven cancellation distinction
Sorry, I don't quite get this comment. In either cases, won't the state still
transit to BackendExecState::CANCELLED ?
--
To view, visit http://gerrit.cloudera.org:8080/11615
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa
Gerrit-Change-Number: 11615
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
