[Impala-ASF-CR] security: only lookup hostname if HOST substitution is required
Hello Alexey Serbin, Kudu Jenkins, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7894 to review the following change. Change subject: security: only lookup hostname if _HOST substitution is required .. security: only lookup hostname if _HOST substitution is required The Kerberos principal configuration uses the special token '_HOST' to indicate that the FQDN of the host should be specified. Previously we would always lookup the FQDN even if the substitution was not required, which might mean that startup would fail if there was no FQDN available, even if no _HOST substitution was required. Now, we only lookup the FQDN if FLAGS_principal contains the substitution token. This provides the possibility of a workaround of explicit principal configuration on machines with no FQDN. Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe Reviewed-on: http://gerrit.cloudera.org:8080/7694 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M be/src/kudu/security/init.cc 1 file changed, 10 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/7894/1 -- To view, visit http://gerrit.cloudera.org:8080/7894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-5854: Update external hadoop versions
Zach Amsden has posted comments on this change. Change subject: IMPALA-5854: Update external hadoop versions .. Patch Set 1: Components are already built, S3 buckets updated, and I ran a private exhaustive build & load job, which passed: http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/6245/console -- To view, visit http://gerrit.cloudera.org:8080/7892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ab9512c3fa3e94c3a4fa9eeb49ff5563a114954 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-1942. Kerberos fails to log in on hostnames with capital letters
Hello Alexey Serbin, Kudu Jenkins, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7893 to review the following change. Change subject: KUDU-1942. Kerberos fails to log in on hostnames with capital letters .. KUDU-1942. Kerberos fails to log in on hostnames with capital letters This ensures that servers canonicalize their FQDNs to lower-case before generating Kerberos principal names. With this change I was able to set up a working cluster on my laptop with a capitalized hostname, where before it would fail as described in the JIRA. I also verified that I was able to connect from both C++ and Java clients. Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627 Reviewed-on: http://gerrit.cloudera.org:8080/7693 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M be/src/kudu/security/init.cc 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/7893/1 -- To view, visit http://gerrit.cloudera.org:8080/7893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-5854: Update external hadoop versions
Zach Amsden has uploaded a new change for review. http://gerrit.cloudera.org:8080/7892 Change subject: IMPALA-5854: Update external hadoop versions .. IMPALA-5854: Update external hadoop versions These versions need to be updated and new versions need to be deployed to S3. Change-Id: I4ab9512c3fa3e94c3a4fa9eeb49ff5563a114954 --- M bin/impala-config.sh 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/7892/1 -- To view, visit http://gerrit.cloudera.org:8080/7892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4ab9512c3fa3e94c3a4fa9eeb49ff5563a114954 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden
[Impala-ASF-CR] IMPALA-4850 [DOCS] Create table "comment comes after "partioned by"
Alex Behm has posted comments on this change. Change subject: IMPALA-4850 [DOCS] Create table "comment comes after "partioned by" .. Patch Set 2: Lars, did you clarify this with Laurel? What to do with this change? -- To view, visit http://gerrit.cloudera.org:8080/7080 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I543ff1dbfe1ab8a7e0a0a668130ab060e3af0a5f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Laurel Hale Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: John Russell Gerrit-Reviewer: Laurel Hale Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2422: Fix escaping in LIKE clause
Alex Behm has abandoned this change. Change subject: IMPALA-2422: Fix escaping in LIKE clause .. Abandoned Needs a more involved solution as discussed in the JIRA. -- To view, visit http://gerrit.cloudera.org:8080/7660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: Iec0f5d99c855ea8d4fe1bcf28c05780ba315034b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-2810: Remove column stats restoration when altering table
Alex Behm has posted comments on this change. Change subject: IMPALA-2810: Remove column stats restoration when altering table .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 2191 Awesome! Nice to get rid of this. http://gerrit.cloudera.org:8080/#/c/7857/1/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: Line 1283: # IMPALA-2810: Error message when moving a partitioned table from one database to another Use a similar comment and test flow as the one from IMPALA-1711. See L763 in this file. I think we should do the exact same testing as in IMPALA-1711, except for a partitioned table. -- To view, visit http://gerrit.cloudera.org:8080/7857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ca7063ca1aa9faceed9568d22740d91b6dc20d3 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Alex Behm Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1172/ -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 4: Code-Review+2 rebase -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5617: Rename tpch nested query files per pattern
Alex Behm has posted comments on this change. Change subject: IMPALA-5617: Rename tpch_nested query files per pattern .. Patch Set 1: I think you also need to change test_tpch_nested_queries.py to use the new file names. Please try to run that test locally. -- To view, visit http://gerrit.cloudera.org:8080/7891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie067b201ae20b4f4c61a98be7ac1ec5a3f8febd8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Wood Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2041: Fix negotiation deadlock
Impala Public Jenkins has submitted this change and it was merged. Change subject: KUDU-2041: Fix negotiation deadlock .. KUDU-2041: Fix negotiation deadlock With N threads in the negotiation threadpool, N or more concurrent client negotiation attempts could starve any incoming server negotiation tasks which used the same threadpool. If the set of negotiation attempts forms a graph with a N cycles, the negotiation could deadlock (at least until the negotiation timeout expires) as all nodes in the system wait for a server request to complete, but all nodes have dedicated all their resources to client requests. Fix: split the server and client tasks into two separate pools. Testing: add a unit test which reproduces the issue, and passes with the fix applied. Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Reviewed-on: http://gerrit.cloudera.org:8080/7177 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins Reviewed-on: http://gerrit.cloudera.org:8080/7742 Reviewed-by: Henry Robinson Tested-by: Impala Public Jenkins --- M be/src/kudu/rpc/messenger.cc M be/src/kudu/rpc/messenger.h M be/src/kudu/rpc/reactor.cc M be/src/kudu/rpc/rpc-test-base.h M be/src/kudu/rpc/rpc-test.cc 5 files changed, 65 insertions(+), 10 deletions(-) Approvals: Impala Public Jenkins: Verified Henry Robinson: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] KUDU-2041: Fix negotiation deadlock
Impala Public Jenkins has posted comments on this change. Change subject: KUDU-2041: Fix negotiation deadlock .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Alex Behm has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Michael Ho has posted comments on this change. Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations .. Patch Set 6: Code-Review+1 Rebase. Carry +1. -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Impala Public Jenkins has submitted this change and it was merged. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. IMPALA-5857: avoid invalid free of hedged read metrics The libHdfs API documents that the output parameter is unchanged on error, therefore we do not need to attempt to free it on error. Testing: The bug only reproduced under stress. I don't know how to trigger this error path yet. Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Reviewed-on: http://gerrit.cloudera.org:8080/7885 Reviewed-by: Sailesh Mukil Reviewed-by: Matthew Jacobs Tested-by: Impala Public Jenkins --- M be/src/runtime/disk-io-mgr-scan-range.cc 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Impala Public Jenkins: Verified Matthew Jacobs: Looks good to me, but someone else must approve Sailesh Mukil: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-1865: Avoid heap allocation for payload slices
Impala Public Jenkins has posted comments on this change. Change subject: KUDU-1865: Avoid heap allocation for payload slices .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1171/ -- To view, visit http://gerrit.cloudera.org:8080/7744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Impala Public Jenkins has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1170/ -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Michael Ho has posted comments on this change. Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations .. Patch Set 5: Code-Review+1 Carry +1 -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Michael Ho has posted comments on this change. Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 53: // Network address of the Impala service on this backend > specify that's the thrift address Done PS4, Line 73: svc_ > maybe just call it krpc_address for consistency e.g. address/krpc_address a Done -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Michael Ho has posted comments on this change. Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations .. Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS4, Line 39: Kudu > still says Kudu. Isn't that confusing since it has nothing to do with kudu OK. Renamed to KRPC to avoid confusion. I guess people may still ask what K stands for in KRPC at some point. Also hiding this flag for now. http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: Line 72: // doesn't have to resolve it on every heartbeat. > would help to update that comment to include KRPC needing resolved IP. Done PS4, Line 231: DCHECK > nit: DCHECK_EQ Done http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 353: // Initiating coordinator. > would be nice to indicate this is the thrift address. Done Line 407: // ... which is being executed on this server > is that the thrift address? would be nice to comment that. Done -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7760 to look at the new patch set (#5). Change subject: IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations .. IMPALA-4856, IMPALA-4872: Include KRPC services in plan fragment's destinations This change allows Impala to publish the IP address and port information of KRPC services if it's enabled via the flag use_krpc. The information is included in a new field in the backend descriptor published as statestore updates. Scheduler will also include this information in the destinations of plan fragments. Also updated the mini-cluster startup script to assign KRPC ports to Impalad instances. This patch also takes into account of a problem found in IMPALA-5795. In particular, the backend descriptor of the coordinator may not be found in the backend map in the scheduler if coordinator is not an executor (i.e. dedicated coordinator). The fix to also check against the local backend descriptor. This patch is partially based on an abandoned patch by Henry Robinson. Testing done: ran core tests with a patch which ignores the use_krpc flag to exercise the code in scheduler. Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/backend-config.cc M be/src/scheduling/backend-config.h M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/util/network-util.cc M be/src/util/network-util.h M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M common/thrift/StatestoreService.thrift 16 files changed, 158 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/5 -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5617: Rename tpch nested query files per pattern
Tim Wood has uploaded a new change for review. http://gerrit.cloudera.org:8080/7891 Change subject: IMPALA-5617: Rename tpch_nested query files per pattern .. IMPALA-5617: Rename tpch_nested query files per pattern The test driver did not pick them up because the name prefix did not match the workload dirname. Change-Id: Ie067b201ae20b4f4c61a98be7ac1ec5a3f8febd8 --- R testdata/workloads/tpch_nested/queries/tpch_nested-q1.test R testdata/workloads/tpch_nested/queries/tpch_nested-q10.test R testdata/workloads/tpch_nested/queries/tpch_nested-q11.test R testdata/workloads/tpch_nested/queries/tpch_nested-q12.test R testdata/workloads/tpch_nested/queries/tpch_nested-q13.test R testdata/workloads/tpch_nested/queries/tpch_nested-q14.test R testdata/workloads/tpch_nested/queries/tpch_nested-q15.test R testdata/workloads/tpch_nested/queries/tpch_nested-q16.test R testdata/workloads/tpch_nested/queries/tpch_nested-q17.test R testdata/workloads/tpch_nested/queries/tpch_nested-q18.test R testdata/workloads/tpch_nested/queries/tpch_nested-q19.test R testdata/workloads/tpch_nested/queries/tpch_nested-q2.test R testdata/workloads/tpch_nested/queries/tpch_nested-q20.test R testdata/workloads/tpch_nested/queries/tpch_nested-q21.test R testdata/workloads/tpch_nested/queries/tpch_nested-q22.test R testdata/workloads/tpch_nested/queries/tpch_nested-q3.test R testdata/workloads/tpch_nested/queries/tpch_nested-q4.test R testdata/workloads/tpch_nested/queries/tpch_nested-q5.test R testdata/workloads/tpch_nested/queries/tpch_nested-q6.test R testdata/workloads/tpch_nested/queries/tpch_nested-q7.test R testdata/workloads/tpch_nested/queries/tpch_nested-q8.test R testdata/workloads/tpch_nested/queries/tpch_nested-q9.test 22 files changed, 0 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/7891/1 -- To view, visit http://gerrit.cloudera.org:8080/7891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie067b201ae20b4f4c61a98be7ac1ec5a3f8febd8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Wood
[Impala-ASF-CR] IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7886/1//COMMIT_MSG Commit Message: PS1, Line 7: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options an impala shell test would be nice too e.g. some test file in tests/shell/ PS1, Line 7: imapla this would be the name of our imap server PS1, Line 40: : The other users of this state, I think HiveServer2's OpenSession() : call and HiveServer2's response to a "SET" query are affected. It seems : like they'd benefit from the same fix, but I've not been able to : adequately run through that code path. you can add a hs2 test in tests/hs2/ , probably test_hs2.py http://gerrit.cloudera.org:8080/#/c/7886/1/be/src/service/query-options.h File be/src/service/query-options.h: PS1, Line 103: that that aren't set and lack defaults PS1, Line 104: considered "unset", which is mapped this is just a bit wordy, I think this makes it more clear: ...are mapped to the empty string -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Joe McDonnell has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7730/5/be/src/util/thread.cc File be/src/util/thread.cc: Line 48: DEFINE_bool(thread_creation_fault_injection, false, "Causes calls to Thread::Create() " > Nice! That makes sense. It is good for it to be close to the other fault injection options. I made this only for DEBUG builds. I think that is sufficient for now. -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7730 to look at the new patch set (#6). Change subject: IMPALA-5750: Catch exceptions from boost thread creation .. IMPALA-5750: Catch exceptions from boost thread creation The boost thread constructor will throw boost::thread_resource_error if it is unable to spawn a thread on the system (e.g. due to a ulimit). This uncaught exception crashes Impala. Systems with a large number of nodes and threads are hitting this limit. This change catches the exception from the thread constructor and converts it to a Status. This requires several changes: 1. util/thread.h's Thread constructor is now private and all Threads are constructed via a new Create() static factory method. 2. util/thread-pool.h's ThreadPool requires that Init() be called after the ThreadPool is constructed. 3. To propagate the Status, Threads cannot be created in constructors, so this is moved to initialization methods that can return Status. 4. Threads now use unique_ptr's for management in all cases. Threads cannot be used as stack-allocated local variables or direct declarations in classes. Query execution code paths will now handle the error: 1. If the scan node fails to spawn any scanner thread, it will abort the query. 2. Failing to spawn a fragment instance from the query state in StartFInstances() will correctly report the error to the coordinator and tear down the query. Testing: This introduces the parameter thread_creation_fault_injection, which will cause Thread::Create() calls in eligible locations to fail randomly roughly 1% of the time. Quite a few locations of Thread::Create() and ThreadPool::Init() are necessary for startup and cannot be eligible. However, all the locations used for query execution are marked as eligible and governed by this parameter. The code was tested by setting this parameter to true and running queries to verify that queries either run to completion with the correct result or fail with appropriate status. Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 --- M be/src/benchmarks/thread-create-benchmark.cc M be/src/catalog/catalog-server.cc M be/src/catalog/catalog-server.h M be/src/catalog/catalogd-main.cc M be/src/common/global-flags.cc M be/src/common/init.cc M be/src/common/init.h M be/src/exec/blocking-join-node.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scan-node.h M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/rpc/TAcceptQueueServer.cpp M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc M be/src/rpc/thrift-server-test.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/rpc/thrift-thread.cc M be/src/rpc/thrift-thread.h M be/src/runtime/data-stream-sender.cc M be/src/runtime/disk-io-mgr-handle-cache.h M be/src/runtime/disk-io-mgr-handle-cache.inline.h M be/src/runtime/disk-io-mgr.cc M be/src/runtime/exec-env.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/parallel-executor.cc M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/thread-resource-mgr.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/service/child-query.cc M be/src/service/child-query.h M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-beeswax-server.cc M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/statestore/statestore-subscriber.cc M be/src/statestore/statestore-subscriber.h M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/util/thread-pool-test.cc M be/src/util/thread-pool.h M be/src/util/thread.cc M be/src/util/thread.h M common/thrift/generate_error_codes.py 55 files changed, 460 insertions(+), 227 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/7730/6 -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Include-what-you-use for Kudu client
Matthew Jacobs has posted comments on this change. Change subject: Include-what-you-use for Kudu client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7872/2//COMMIT_MSG Commit Message: Line 7: Include-what-you-use for Kudu client since this may need to be tracked for backports, please file a JIRA for this even though it's trivial -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5210: Count rows and collection items in parquet scanner separately
Tianyi Wang has posted comments on this change. Change subject: IMPALA-5210: Count rows and collection items in parquet scanner separately .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7776/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS5, Line 963: coll_items_read_counter_ > does that need to be a field in the object, given that it's reset ever time If we pass it through function calls about 10 function signatures would be affected. Patch 1 would be what it looks like. Is there a better way to do it? -- To view, visit http://gerrit.cloudera.org:8080/7776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7f6efddaea18507482940f5bdab7326b6482b067 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tianyi Wang Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Rename thrift-deps to gen-deps
Impala Public Jenkins has submitted this change and it was merged. Change subject: IMPALA-4856: Rename thrift-deps to gen-deps .. IMPALA-4856: Rename thrift-deps to gen-deps As a preparation to start generating Protobuf files for IMPALA-4856, this change introduces a new build target "gen-deps" which serves as an umbrella for all build targets of generated code. For now, it only includes thrift-deps and protobuf targets will be added in the future. Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9 Reviewed-on: http://gerrit.cloudera.org:8080/7851 Reviewed-by: Lars Volker Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M CMakeLists.txt M be/src/catalog/CMakeLists.txt M be/src/codegen/CMakeLists.txt M be/src/common/CMakeLists.txt M be/src/exec/CMakeLists.txt M be/src/experiments/CMakeLists.txt M be/src/exprs/CMakeLists.txt M be/src/rpc/CMakeLists.txt M be/src/runtime/CMakeLists.txt M be/src/runtime/bufferpool/CMakeLists.txt M be/src/scheduling/CMakeLists.txt M be/src/service/CMakeLists.txt M be/src/statestore/CMakeLists.txt M be/src/testutil/CMakeLists.txt M be/src/transport/CMakeLists.txt M be/src/udf/CMakeLists.txt M be/src/udf_samples/CMakeLists.txt M be/src/util/CMakeLists.txt M ext-data-source/CMakeLists.txt 19 files changed, 33 insertions(+), 30 deletions(-) Approvals: Impala Public Jenkins: Verified Lars Volker: Looks good to me, but someone else must approve Tim Armstrong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4856: Rename thrift-deps to gen-deps
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4856: Rename thrift-deps to gen-deps .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5210: Count rows and collection items in parquet scanner separately
Dan Hecht has posted comments on this change. Change subject: IMPALA-5210: Count rows and collection items in parquet scanner separately .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7776/5/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: PS5, Line 963: coll_items_read_counter_ does that need to be a field in the object, given that it's reset ever time through? The counter code is already over complicated, but whatever we can do to not add complexity would be good. -- To view, visit http://gerrit.cloudera.org:8080/7776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7f6efddaea18507482940f5bdab7326b6482b067 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tianyi Wang Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5653: Remove "unlimited" process mem limit option
Dan Hecht has posted comments on this change. Change subject: IMPALA-5653: Remove "unlimited" process mem_limit option .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7828 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifb235ae34ce8d2aff37f0fa0c218419da01b30f3 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2041: Fix negotiation deadlock
Impala Public Jenkins has posted comments on this change. Change subject: KUDU-2041: Fix negotiation deadlock .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1169/ -- To view, visit http://gerrit.cloudera.org:8080/7742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation .. Patch Set 5: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/7730/5/be/src/util/thread.cc File be/src/util/thread.cc: Line 48: DEFINE_bool(thread_creation_fault_injection, false, "Causes calls to Thread::Create() " Nice! It seems like this should probably be grouped with other stress options in common/global-flags.cc. Those are also only enabled for debug builds. I guess it could be interesting to test on release builds since the timing will be different but ensure if it's worth making this a special case. -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
Philip Zeyliger has uploaded a new change for review. http://gerrit.cloudera.org:8080/7886 Change subject: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options .. IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options When converting TQueryOptions to a map, we now convert unset options to the empty string. Within TQueryOptions we have optional options (like mt_dop or compression_codec) with no default specified. In this case, the user was seeing 0 for numeric types and the first enum option for enumeration types (e.g., "NONE" in the compression case.). This was confusing as the implementation handles this "null" case differently (e.g., using SNAPPY as the default codec in the case reported in the JIRA). When running "set" in impala-shell, the difference is as follows: -BUFFER_POOL_LIMIT: [0] +BUFFER_POOL_LIMIT: [] -COMPRESSION_CODEC: [NONE] +COMPRESSION_CODEC: [] -MT_DOP: [0] +MT_DOP: [] -RESERVATION_REQUEST_TIMEOUT: [0] +RESERVATION_REQUEST_TIMEOUT: [] -SEQ_COMPRESSION_MODE: [0] +SEQ_COMPRESSION_MODE: [] -V_CPU_CORES: [0] +V_CPU_CORES: [] Obviously, the empty string is a valid value for a string-typed option, where it will be impossible to tell the difference between "unset" and "set to empty string." Today, there are two string-typed options: debug_string defaults to "" and request_pool has no default. An alternative would have been to use a special token like "_unset" or to introduce a new field in the beeswax Thrift ConfigVariable struct. The other users of this state, I think HiveServer2's OpenSession() call and HiveServer2's response to a "SET" query are affected. It seems like they'd benefit from the same fix, but I've not been able to adequately run through that code path. Change-Id: I86bc06a58d67b099da911293202dae9e844c439b --- M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/service/query-options.h 3 files changed, 22 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/7886/1 -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1168/ -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics; > It just calls a free(ptr): Yeah exactly. -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics; > I think it's coincidental that hdfsFreeHedgedReadMetrics() currently works It just calls a free(ptr): http://github.mtv.cloudera.com/CDH/hadoop/blob/9ffaadf806c7a061f3494ebdcaec9d43da24b2bf/hadoop-hdfs-project/hadoop-hdfs/src/main/native/libhdfs/hdfs.c#L174 But I agree that it's better to rely on the documented interface rather than the internal implementation that could change at any point. -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5317: add DATE TRUNC() function
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5317: add DATE_TRUNC() function
..
Patch Set 3: Code-Review+1
(1 comment)
LGTM but would like someone else to check I didn't miss any edge cases
http://gerrit.cloudera.org:8080/#/c/7313/2/be/src/exprs/udf-builtins.cc
File be/src/exprs/udf-builtins.cc:
Line 260: }
> Done. Its already inside the the namespace {} block.
Ahh I see. Didn't notice that.
--
To view, visit http://gerrit.cloudera.org:8080/7313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: sandeep akinapelli
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics; > Would it be less confusing if we just initialized 'hedged_metrics' to NULL I think it's coincidental that hdfsFreeHedgedReadMetrics() currently works if the argument is a NULL pointer. It's not documented in the interface. Seems like a bad idea to depend on the behaviour. -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Impala Public Jenkins has submitted this change and it was merged. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test Add a targeted test that confirms that setting the query option will force spilling. Testing: Ran test_spilling locally. Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Reviewed-on: http://gerrit.cloudera.org:8080/7809 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- D testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test A testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test M tests/common/impala_test_suite.py M tests/query_test/test_spilling.py 4 files changed, 87 insertions(+), 25 deletions(-) Approvals: Impala Public Jenkins: Verified Tim Armstrong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5317: add DATE TRUNC() function
sandeep akinapelli has posted comments on this change.
Change subject: IMPALA-5317: add DATE_TRUNC() function
..
Patch Set 2:
(12 comments)
Addressed review comments.
http://gerrit.cloudera.org:8080/#/c/7313/2/be/src/exprs/udf-builtins.cc
File be/src/exprs/udf-builtins.cc:
PS2, Line 52: untils
> units?
Done
Line 245: // fractional seconds are nanoseconds as per the build configuration
> This comment is a bit cryptic - not clear which build configuration it mean
Done
Line 254: // fractional seconds are nanoseconds as per the build
configuration.
> Same here.
Done
Line 260: // used by both Trunc and DateTrunc functions to perform the
truncation
> Put doTrunc() in an anonymous namespace - "namespace {" - to avoid avoid e
Done. Its already inside the the namespace {} block.
PS2, Line 261: doTrunc
> nit: casing should be DoTrunc().
Done
Line 267: static const date week_limit_date(1400, 1, 6);
> local static variables have weird semantics - it would be best to avoid the
Done
PS2, Line 272: // for millenium < 2000 year value goes to 1000
: // (outside impala supported range)
> nit: comment fits on one line.
Done
Line 275: if (orig_date.is_special()) return TimestampVal::null();
> Shouldn't we check whether the date is special before checking the year? I'
Yes. you are correct. switched the ifs.
Line 280: if (orig_date.is_special()) return TimestampVal::null();
> Same here - should we check special first?
Done
Line 289: // used by DateTrunc only
> This comment seems unnecessary (it's documented in the enum).
Done
Line 296: // used by DateTrunc only
> Same here.
Done
Line 334: // used by DateTrunc only
> Same here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: sandeep akinapelli
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5317: add DATE TRUNC() function
sandeep akinapelli has uploaded a new patch set (#3). Change subject: IMPALA-5317: add DATE_TRUNC() function .. IMPALA-5317: add DATE_TRUNC() function Added a UDF builtin function date_trunc. Reuse many of the Trunc functions implemented already for trunc() including truncate unit and except strToTruncUnit Added checks to ensure that truncation results that fall outside of posix timestamp range are returned as NULL. Added ctest for the date_trunc function. Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584 --- M be/src/exprs/expr-test.cc M be/src/exprs/udf-builtins-ir.cc M be/src/exprs/udf-builtins.cc M be/src/exprs/udf-builtins.h M common/function-registry/impala_functions.py 5 files changed, 383 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/7313/3 -- To view, visit http://gerrit.cloudera.org:8080/7313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I953ba006cbb166dcc78e8c0c12dfbf70f093b584 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: sandeep akinapelli Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: sandeep akinapelli
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc File be/src/runtime/disk-io-mgr-scan-range.cc: PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics; Would it be less confusing if we just initialized 'hedged_metrics' to NULL and left hdfsFreeHedgedReadMetrics() where it was? -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Tim Armstrong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7885 Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. IMPALA-5857: avoid invalid free of hedged read metrics The libHdfs API documents that the output parameter is unchanged on error, therefore we do not need to attempt to free it on error. Testing: The bug only reproduced under stress. I don't know how to trigger this error path yet. Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 --- M be/src/runtime/disk-io-mgr-scan-range.cc 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/7885/1 -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Tim Armstrong has uploaded a new patch set (#3). Change subject: IMPALA-5855: reserve enough memory for preaggs .. IMPALA-5855: reserve enough memory for preaggs The calculation in the planner failed to account for the behaviour of Suballocator, which needs to obtain at least one buffer to allocate any memory. Testing: Added a regression test that caused a crash before the fix. Updated planner tests. Was able to run local stress test binary search to completion (it previously crashed). Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b --- M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test 6 files changed, 78 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/7871/3 -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5850: Cast sender partition exprs under unions.
Alex Behm has uploaded a new change for review. http://gerrit.cloudera.org:8080/7884 Change subject: IMPALA-5850: Cast sender partition exprs under unions. .. IMPALA-5850: Cast sender partition exprs under unions. For a series of partitioned joins within the same fragment we must cast the sender partition exprs of exchanges to compatible types. Otherwise, the hashes generated for identical partition values may differ among senders leading to wrong results. The bug was that this casting process was only performed for fragments that are hash-partitioned. However, a union produces a fragment with RANDOM partition, but the union could still contain partitioned joins whose senders need to be cast appropriately. The fix is to add casts regardless of the fragment's data partition. Testing: - Core/hdfs run passed - Added a new regresion test Change-Id: I0aa801bcad8c2324d848349c7967d949224404e0 --- M fe/src/main/java/org/apache/impala/planner/PlanFragment.java M testdata/workloads/functional-query/queries/QueryTest/joins.test 2 files changed, 83 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/7884/1 -- To view, visit http://gerrit.cloudera.org:8080/7884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0aa801bcad8c2324d848349c7967d949224404e0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 2: Fixed a couple of tests -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Include-what-you-use for Kudu client
Thomas Tauber-Marshall has uploaded a new patch set (#2). Change subject: Include-what-you-use for Kudu client .. Include-what-you-use for Kudu client A recent commit in Kudu removed some unnecessary includes from Kudu client headers to reduce compile times. We were implicitly relying on those includes, so before we upgrade to a newer version of the client we need to make the includes explicit on our side to avoid breaking compilation. Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce --- M be/src/exec/kudu-table-sink.cc M be/src/exec/kudu-util.cc M be/src/exprs/kudu-partition-expr.h 3 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/7872/2 -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] Include-what-you-use for Kudu client
Thomas Tauber-Marshall has posted comments on this change. Change subject: Include-what-you-use for Kudu client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7872/1/be/src/exprs/kudu-partition-expr.h File be/src/exprs/kudu-partition-expr.h: PS1, Line 21: #include : #include > can you try to just forward declare instead of including here (then includi That gives an error of the form: 'unique_ptr.h:74:22: error: invalid application of ‘sizeof’ to incomplete type ‘kudu::KuduPartialRow’' -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] Rough draft for IMPALA-5628
Bikramjeet Vig has posted comments on this change.
Change subject: Rough draft for IMPALA-5628
..
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 1254: // where to put that check? refer dcheck added below
> This could possibly go with the other metadata checks, after initializing t
makes sense, will do
http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:
Line 89: switch(parquet_type){
> This switch on the parquet type looks like it may fit better in the Parquet
Do you mean to have a Decode wrapper around the templatized Decode methods? or
just keep the current single templated Decode and switch on parquet_type inside
each specialized Decode?
http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h
File be/src/exec/parquet-common.h:
PS2, Line 237: ByteSize
> These calls could now be removed and replaced with the bytesize derived fro
Do you mean to have something like
- template int
ParquetPlainEncoder::ByteSize() , and then use overloading to select which type
is passed, not sure how much perf impact will there be for overloading here
OR
- template int
ParquetPlainEncoder::ByteSize()
Line 338: template <>
> Can this be deduplicated?
thats kinda difficult because DecimalUtil::DecodeFromFixedLenByteArray is also
templated and extracting the code before that into a common inlined function is
not worth it because we need the 3 variables defined there in our call to
DecodeFromFixedLenByteArray and to return, so would have pass 3 pointers to
that method (and probably call it init or something like that) which might make
the code ugly
--
To view, visit http://gerrit.cloudera.org:8080/7822
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] Rough draft for IMPALA-5628
Bikramjeet Vig has uploaded a new change for review. http://gerrit.cloudera.org:8080/7822 Change subject: Rough draft for IMPALA-5628 .. Rough draft for IMPALA-5628 Request for general comments on approch. Many changes pending, will keep updating. Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e --- M be/src/exec/data-source-scan-node.cc M be/src/exec/hdfs-parquet-scanner.h M be/src/exec/parquet-column-readers.cc M be/src/exec/parquet-column-stats.cc M be/src/exec/parquet-column-stats.h M be/src/exec/parquet-column-stats.inline.h M be/src/exec/parquet-common.h M be/src/exec/parquet-metadata-utils.cc M be/src/exec/parquet-plain-test.cc M be/src/runtime/types.cc M be/src/runtime/types.h M be/src/util/dict-encoding.h 12 files changed, 255 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/7822/2 -- To view, visit http://gerrit.cloudera.org:8080/7822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Philip Zeyliger has uploaded a new patch set (#3). Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.) .. IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.) This commit simplifies the following conditional functions: * istrue * isfalse * isnottrue * isnotfalse * zeroifnull * nullifzero * nullvalue * nonnullvalue * nullif Most of these simplifications work when the arguments to these functions are literals. The one exception is that nullif(x, y) simplifies to NULL when x.equals(y) as expressions. This is valid since nullif(null, null)=null. This simplification has not been applied to the equivalent "case x=x then null else null". In terms of the implementation is a bit of a toss-up between re-writing these into their equivalent case statements or simplifying them here. Testing: * Added new tests to ExprRewriteRulesTest * Confirmed (using EclEmma, which uses jococo engine) that coverage is good. Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5 --- M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java 2 files changed, 208 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/3 -- To view, visit http://gerrit.cloudera.org:8080/7829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
..
Patch Set 2:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS2, Line 145: /**
:* Helper for simplifying unary boolean function (like
istrue(x)).
:*/
: private Expr unaryHelper(Expr expr, LiteralExpr child,
: boolean onTrue, boolean onFalse, boolean onNull)
> I'm not crazy about this interface because I had to read all the code to re
I added a few more comments here. I don't have a better name idea.
"isBooleanHelper()" seems no better.
I moved this below the code that uses it so it reads easier for reviewers.
PS2, Line 153: Boolean
> is there a reason you use Boolean over boolean? Given getValue() returns th
Should have been boolean; fixed by inlining.
PS2, Line 154: val
> also, I don't see a reason not to getValue() inline here
Done
PS2, Line 165:
: /**
:* Helper for simplifying unary functions like isnull(x).
:*/
: private Expr unaryHelper2(Expr expr, LiteralExpr child,
boolean onNull,
: boolean onOther)
> same
I got rid of this method entirely.
Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr,
Analyzer analyzer) throws AnalysisException {
> nit: long line, please wrap at 90
Done
PS2, Line 197: //
ontrue onfalse onnull
: if (fnName.equals("isfalse"))return
unaryHelper(expr, c, false, true, false);
> nit: we typically don't use cool whitespace formatting to line things up. I
I debated using whitespace here vs. doing the easy thing, and I decided that it
was a little bit easier to read when the truth table is right here in front of
you. I'm happy to be overridden and just to have Eclipse re-format this, but I
do think it's easier to read this way.
I also thought about doing crazy bit math and nested ternary operators. I
didn't go that route, as I figured I couldn't read it.
The implementation for the C++ half of these is
in be/src/exprs/conditional-functions-ir.cc. I looked and I could obviously
write four functions to parallel that implementation, but it seems no better.
PS2, Line 235: x
> y
Done
Line 237:* Note that we currently don't simplify all possible equal
expressions
> This may be obvious, but not to me. Would you mind elaborating? I understan
Updated the comment.
Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer
analyzer) throws AnalysisException {
> nit long line
Done
PS2, Line 248: literalExprsEqual
> can you explain why this does constant folding but other optimizations do n
Done
PS2, Line 300: Rewrites a = b
> Doesn't indicate that this creates Expr 'a = b'. How about:
Done
PS2, Line 303: literal
> this fn doesn't appear to require literal exprs. A more precise name might
Done
PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred,
analyzer);
: return rewritten;
> remove local var and return result
Sure. I think I default to being more verbose because I like to set
breakpoints, which leads me to having one thing per line.
PS2, Line 315: return rewritten instanceof BoolLiteral &&
: ((BoolLiteral) rewritten).getValue();
> nit 1line? It looks less than 90chars...
Done
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Philip Zeyliger
Gerrit-HasComments: Yes
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Sailesh Mukil has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: Code-Review+2 Thanks for the explanations. This LGTM -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Rename thrift-deps to gen-deps
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4856: Rename thrift-deps to gen-deps .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1167/ -- To view, visit http://gerrit.cloudera.org:8080/7851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2041: Fix negotiation deadlock
Hello Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7742 to look at the new patch set (#2). Change subject: KUDU-2041: Fix negotiation deadlock .. KUDU-2041: Fix negotiation deadlock With N threads in the negotiation threadpool, N or more concurrent client negotiation attempts could starve any incoming server negotiation tasks which used the same threadpool. If the set of negotiation attempts forms a graph with a N cycles, the negotiation could deadlock (at least until the negotiation timeout expires) as all nodes in the system wait for a server request to complete, but all nodes have dedicated all their resources to client requests. Fix: split the server and client tasks into two separate pools. Testing: add a unit test which reproduces the issue, and passes with the fix applied. Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Reviewed-on: http://gerrit.cloudera.org:8080/7177 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins --- M be/src/kudu/rpc/messenger.cc M be/src/kudu/rpc/messenger.h M be/src/kudu/rpc/reactor.cc M be/src/kudu/rpc/rpc-test-base.h M be/src/kudu/rpc/rpc-test.cc 5 files changed, 65 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7742/2 -- To view, visit http://gerrit.cloudera.org:8080/7742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-2234: remove workaround from stress test
Tim Armstrong has submitted this change and it was merged. Change subject: IMPALA-2234: remove workaround from stress test .. IMPALA-2234: remove workaround from stress test We believe that IMPALA-2234 has probably been fixed at some point - either by IMPALA-3200 or other query lifecycle changes. Let's remove the workaround from the stress test script to confirm that it's no longer needed. Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Reviewed-on: http://gerrit.cloudera.org:8080/7874 Reviewed-by: Lars Volker Reviewed-by: Dan Hecht Tested-by: Tim Armstrong --- M tests/stress/concurrent_select.py 1 file changed, 1 insertion(+), 3 deletions(-) Approvals: Lars Volker: Looks good to me, but someone else must approve Tim Armstrong: Verified Dan Hecht: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-2234: remove workaround from stress test
Tim Armstrong has posted comments on this change. Change subject: IMPALA-2234: remove workaround from stress test .. Patch Set 1: Verified+1 Code is not exercised by precommit tests. Ran a local stress test to make sure I didn't break anythin. -- To view, visit http://gerrit.cloudera.org:8080/7874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Rename thrift-deps to gen-deps
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4856: Rename thrift-deps to gen-deps .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I360c63773efdeab4c26ca96b915e0c8d0ce2b9c9 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2041: Fix negotiation deadlock
Henry Robinson has posted comments on this change. Change subject: KUDU-2041: Fix negotiation deadlock .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7742 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38379eeaf7516d432708c2a2a285839f96c86d4f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-1865: Avoid heap allocation for payload slices
Dan Hecht has posted comments on this change. Change subject: KUDU-1865: Avoid heap allocation for payload slices .. Patch Set 1: Code-Review+2 I didn't look closely, but if this is a clean "cherry pick" of the fix from kudu, I think we should go ahead and merge it. -- To view, visit http://gerrit.cloudera.org:8080/7744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Dan Hecht has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS4, Line 39: Kudu still says Kudu. Isn't that confusing since it has nothing to do with kudu (other than being derived from it)? Maybe just call it "KRPC". Or should we make this flag hidden so users don't find it until the feature is finished? http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: Line 72: // doesn't have to resolve it on every heartbeat. would help to update that comment to include KRPC needing resolved IP. PS4, Line 231: DCHECK nit: DCHECK_EQ http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 353: // Initiating coordinator. would be nice to indicate this is the thrift address. Line 407: // ... which is being executed on this server is that the thrift address? would be nice to comment that. http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 53: // Network address of the Impala service on this backend specify that's the thrift address PS4, Line 73: svc_ maybe just call it krpc_address for consistency e.g. address/krpc_address and server/krpc_server naming. the 'address' field is also that of a "service". okay to ignore if you feel this naming is better. -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Henry Robinson has posted comments on this change. Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 .. Patch Set 4: (1 comment) Also fix a bug in how the options are checked after setting. http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch: PS4, Line 35: SSL_OP_NO_TLSv1_2 0x0800L > Thanks for the explanation. I think it would be good to add a comment to th I think that comment would just explain in words what the code does - it's pretty clear that if the return value from SSL_CTX_set_options isn't what we expect, we throw an error. -- To view, visit http://gerrit.cloudera.org:8080/7859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e Gerrit-PatchSet: 4 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7859 to look at the new patch set (#5). Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 .. IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 Thrift commit taken from: https://github.com/henryr/thrift/commit/d828da9 "This patch slightly changes the TLS configuration code to remove the compile-time checks that were used to disable 1.1+ support based on the OpenSSL library on the build machine. Instead, we define the symbols we need ourselves if necessary, and rely on runtime behaviour to catch errors or unsupported configurations." Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e --- M buildall.sh A source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch 2 files changed, 93 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/59/7859/5 -- To view, visit http://gerrit.cloudera.org:8080/7859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e Gerrit-PatchSet: 5 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-2234: remove workaround from stress test
Dan Hecht has posted comments on this change. Change subject: IMPALA-2234: remove workaround from stress test .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 4: Code-Review+2 rebase -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1166/ -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Michael Brown has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Hello Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7809 to look at the new patch set (#3). Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test Add a targeted test that confirms that setting the query option will force spilling. Testing: Ran test_spilling locally. Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce --- D testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test A testdata/workloads/functional-query/queries/QueryTest/spilling-query-options.test M tests/common/impala_test_suite.py M tests/query_test/test_spilling.py 4 files changed, 87 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/7809/3 -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 3: Code-Review+1 carry -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/7809/2/tests/query_test/test_spilling.py File tests/query_test/test_spilling.py: PS2, Line 33: TestSpilling > Does it make sense to call this TestSpillingDebugActionDimensions ? Works for me. PS2, Line 71: TestSpillingNoDebugAction > Maybe append Dimensions to this name? At least one test uses debug actions, Done PS2, Line 95: setting debug_action to alternative values > Maybe append the comment "via query options". Done -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Michael Ho has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS3, Line 39: Kudu RPC > that seems confusing since it has nothing to do with Kudu from the users' p Done http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h File be/src/scheduling/backend-config.h: PS3, Line 60: hostname > if it's a host name, why is the type TNetworkAddress as opposed to Hostname This is needed to compare the port too. Apparently, each hostname maps to a list of backend descriptor. We return the first descriptor with matching IP and port. http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: PS3, Line 95: > do we have a check anywhere that this flag is set, and give an error messag It's set to default value 29000 if the user doesn't specify it during startup. We can potentially do a check for the validity of its value but I don't see the need to make an exception for it if we aren't already testing the values of other ports or whether the port is already in use. Line 241: ADD_TIMER(schedule->summary_profile(), "ComputeScanRangeAssignmentTimer"); > isn't this dcheck really saying that if LookupBackendDesc() returned nullpt Done. Note that in release builds, we will just return the wrong descriptor. Is it better to crash or should we trust our testing to have covered it ? I suppose similar questions can be asked about other DCHECKs in the existing code base. Line 711: #endif > I don't understand that. isn't this just copying the shared_ptr, not the Ba The way the update happens is that it overwrites the shared_ptr "executors_config_" atomically to point to the new copy. So, if you call GetExecutorsConfig() twice, you can potentially get back two different pointers. Checking out a copy here (with shared_ptr) makes sure that we retain reference to the old copy even after "executors_config_" has been overwritten. http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 410: // IP address + port of the KRPC based services on the destination > does this have to be resolved IP as well? Done http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 72: // IP address + port of KRPC based services on this backend > this one has to be a resolved IP, right? let's make that clear. Done -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7760 to look at the new patch set (#4). Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations .. IMPALA-4856: Include KRPC services in plan fragment's destinations This change allows Impala to publish the address and port information of KRPC services if it's enabled via the flag use_krpc. The information is included in a new field in the backend descriptor published as statestore updates. Scheduler will also include this information in the destinations of plan fragments. Also updated the mini-cluster startup script to assign KRPC ports to Impalad instances. This patch also takes into account of a problem found in IMPALA-5795. In particular, the backend descriptor of the coordinator may not be found in the backend map in the scheduler if coordinator is not an executor (i.e. dedicated coordinator). The fix to also check against the local backend descriptor. This patch is partially based on an abandoned patch by Henry Robinson. Testing done: ran core tests with a patch which ignores the use_krpc flag to exercise the code in scheduler. Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 --- M be/src/common/global-flags.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/backend-config.cc M be/src/scheduling/backend-config.h M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h M be/src/service/impala-server.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/util/network-util.cc M be/src/util/network-util.h M bin/start-impala-cluster.py M common/thrift/ImpalaInternalService.thrift M common/thrift/StatestoreService.thrift 16 files changed, 149 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/7760/4 -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4826 Fix wrong scan result on repeated root schema in Parquet.
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4826 Fix wrong scan result on repeated root schema in Parquet. .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7870/1//COMMIT_MSG Commit Message: Line 9: Having the repetition level set to REPEATED on the root schema > Can you explain why it should be ignored, i.e. include a reference to the p Yeah it would be good to reference the PARQUET JIRA. http://gerrit.cloudera.org:8080/#/c/7870/1/testdata/data/README File testdata/data/README: Line 111: Generated by hacking Impala's Parquet writer. > I'm not very happy with us collecting more and more specially crafted files If it's just a one or two line change we could include the diff inline here. -- To view, visit http://gerrit.cloudera.org:8080/7870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ea84589e1d122ad9d43adde46893ec0ecc5f9c4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2234: remove workaround from stress test
Lars Volker has posted comments on this change. Change subject: IMPALA-2234: remove workaround from stress test .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Mulder Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 2: > I think you may need to update test admission-reject-min-reservation.test now > too, since that went in last night Works for me locally -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2234: remove workaround from stress test
Tim Armstrong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7874 Change subject: IMPALA-2234: remove workaround from stress test .. IMPALA-2234: remove workaround from stress test We believe that IMPALA-2234 has probably been fixed at some point - either by IMPALA-3200 or other query lifecycle changes. Let's remove the workaround from the stress test script to confirm that it's no longer needed. Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e --- M tests/stress/concurrent_select.py 1 file changed, 1 insertion(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/7874/1 -- To view, visit http://gerrit.cloudera.org:8080/7874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic154ce245195e8ef9af136da7ab58e82b5b4354e Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0 .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7866/1//COMMIT_MSG Commit Message: Line 25: Could you add in the commit message what kind of error messages to expect when using unsupported TLS protocol versions with an older OpenSSL? It will be useful to lookup and point to if any users have this problem in the future, since you mentioned that the error messages aren't very clear in the JIRA. http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS1, Line 185: system OpenSSL "linked OpenSSL library" for clarity? Should users choose to link it to a non-system library. -- To view, visit http://gerrit.cloudera.org:8080/7866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has abandoned this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Abandoned Will re-upload a new patch once KUDU-2110 is fixed. -- To view, visit http://gerrit.cloudera.org:8080/7747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Michael Ho has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: Yes, we probably need to abandon this and re-upload a new patch once the fix for KUDU-2110 is incorporated into this patch. -- To view, visit http://gerrit.cloudera.org:8080/7747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
..
Patch Set 1: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/thirdparty/squeasel/squeasel.c
File be/src/thirdparty/squeasel/squeasel.c:
Line 4259: SSL_CTX_set_options(ctx->ssl_ctx, options);
Check the return value of this?
if (!(SSL_CTX_set_options(ctx_, options) & options)) {
cry()
}
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/util/webserver-test.cc
File be/src/util/webserver-test.cc:
PS1, Line 298: 0x10001000L
May be define it as OPENSSL_MIN_VERSION_WITH_TLS_1_1 for readability.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4826 Fix wrong scan result on repeated root schema in Parquet.
Lars Volker has posted comments on this change. Change subject: IMPALA-4826 Fix wrong scan result on repeated root schema in Parquet. .. Patch Set 1: (3 comments) Looks good to me, only minor comments http://gerrit.cloudera.org:8080/#/c/7870/1//COMMIT_MSG Commit Message: Line 7: IMPALA-4826 Fix wrong scan result on repeated root schema in Parquet. nit: Colon after JIRA Line 9: Having the repetition level set to REPEATED on the root schema Can you explain why it should be ignored, i.e. include a reference to the parquet format? http://gerrit.cloudera.org:8080/#/c/7870/1/testdata/data/README File testdata/data/README: Line 111: Generated by hacking Impala's Parquet writer. I'm not very happy with us collecting more and more specially crafted files without a way to repro them. Can you push the hack to somewhere, e.g. your public Github, and mention the link here. That way we have a chance of preserving the information. What do others think? -- To view, visit http://gerrit.cloudera.org:8080/7870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ea84589e1d122ad9d43adde46893ec0ecc5f9c4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Bump Kudu version to 1c70e5d
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 1c70e5d .. Patch Set 1: (1 comment) does the code not compile without the header changes? It's best to separate code changes from toolchain version bumps when possible because it makes porting to other branches harder. It's OK if it's not possible. http://gerrit.cloudera.org:8080/#/c/7872/1/be/src/exprs/kudu-partition-expr.h File be/src/exprs/kudu-partition-expr.h: PS1, Line 21: #include : #include can you try to just forward declare instead of including here (then including in the .cc)? for both kudu::client::KuduPartitioner and kudu::KuduPartialRow? I think that a unique_ptr doesn't need the full class definition. -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Michael Ho has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/connection.cc File be/src/kudu/rpc/connection.cc: PS1, Line 269: car->call.reset(); > How do we ensure that we don't do this while the call is sending? Are we re Yes, it's an implicit assumption in Kudu RPC to map the same connection to the same reactor thread. Please see Messenger::RemoteToReactor(const Sockaddr &remote); PS1, Line 626: has already timed out or has already been cancelled > I think at some point, it would be good to differentiate between the two fo If you look at the OutboundCall::SetTimedOut() and OutboundCall::SetCancelled(), we do include different status details for the two different cases. http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/proxy.cc File be/src/kudu/rpc/proxy.cc: Line 85: controller->SetMessenger(messenger_.get()); > The following would be a bug for other reasons, but I just want to confirm This function is expected to be invoked from the same thread which calls cancellation. My understanding is that DSS (or generally speaking the execution thread of a fragment instance) is single threaded. This will be invoke from KrpcDataStreamSender::Close() to cancel any in-flight RPC. We rely on rpc_in_flight_ as a synchronization and it's protected by a lock. Cancellation needs to hold the lock, check rpc_in_flight_ is true before requesting cancellation. DSS is mostly single threaded so the only race is with the reactor thread call-back. The window in which this problem can happen is when we need to reset the RPC controller to retry the RPC (arguably, the reset may not be necessary) but that will happen under a lock anyway. There are actually DCHECKs to make sure call_ and messenger_ aren't nullptr in RpcController::Cancel(). -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 .. Patch Set 4: Code-Review+2 (1 comment) LGTM, just need to add a comment as mentioned below, so it's clear while reading the code. http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch: PS4, Line 35: SSL_OP_NO_TLSv1_2 0x0800L > SSL_CTX_set_options returns the options that are set after that call takes Thanks for the explanation. I think it would be good to add a comment to that effect at L81. -- To view, visit http://gerrit.cloudera.org:8080/7859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e Gerrit-PatchSet: 4 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Henry Robinson has posted comments on this change. Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch: PS4, Line 35: SSL_OP_NO_TLSv1_2 0x0800L > I'm a tad bit confused; if OpenSSL can't understand these options, and we p SSL_CTX_set_options returns the options that are set after that call takes effect. So if you & those with the bitmask you passed in, you can tell whether or not the options took effect. See line 81 for how that's handled. -- To view, visit http://gerrit.cloudera.org:8080/7859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e Gerrit-PatchSet: 4 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5849: Disable compile-time checks for OpenSSL > 1.0.0 .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7859/4/source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch File source/thrift/thrift-0.9.0-patches/0011-CLD-Patch-11-IMPALA-5849-Don-t-use-compile-time-chec.patch: PS4, Line 35: SSL_OP_NO_TLSv1_2 0x0800L I'm a tad bit confused; if OpenSSL can't understand these options, and we pass them into SSL_CTX_set_options(), what's the expected behavior? Ideally it would be to return with an error (as opposed to undefined behavior), but have we tested that's what happens? -- To view, visit http://gerrit.cloudera.org:8080/7859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I437703b2221cba6a2c3079c540ccbda571f8db0e Gerrit-PatchSet: 4 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout
Sailesh Mukil has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout .. Patch Set 2: We decided to abandon this for now, until the bug is fixed right? -- To view, visit http://gerrit.cloudera.org:8080/7747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Michael Ho has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: Did you mean KUDU-2011 ? That's addressed in a follow-up patch. -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] Bump Kudu version to 1c70e5d
Thomas Tauber-Marshall has uploaded a new change for review. http://gerrit.cloudera.org:8080/7872 Change subject: Bump Kudu version to 1c70e5d .. Bump Kudu version to 1c70e5d This required adding a few extra includes due to a recent commit in Kudu that rearranged includes to reduce compile times. Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce --- M be/src/exec/kudu-table-sink.cc M be/src/exec/kudu-util.cc M be/src/exprs/kudu-partition-expr.h M bin/impala-config.sh 4 files changed, 5 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/7872/1 -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Henry Robinson has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: Wasn't there an outstanding bug with cancellation that was found in Kudu? Does that affect Impala? Has it been fixed in this patch? -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Sailesh Mukil has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Sailesh Mukil has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call .. Patch Set 1: (3 comments) Sorry for the delay, I wanted to take some time to understand this well. I just have a few clarifying questions to just ensure that we've thought through this thoroughly. I'm not proposing any code changes at this point. http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/connection.cc File be/src/kudu/rpc/connection.cc: PS1, Line 269: car->call.reset(); How do we ensure that we don't do this while the call is sending? Are we relying on the fact that the Cancel() and Send() will be processed by the same reactor thread and therefore, they can't race? PS1, Line 626: has already timed out or has already been cancelled I think at some point, it would be good to differentiate between the two for debuggability reasons, so we know the difference between an RPC level timeout vs an application level timeout (if we choose to add one) http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/proxy.cc File be/src/kudu/rpc/proxy.cc: Line 85: controller->SetMessenger(messenger_.get()); The following would be a bug for other reasons, but I just want to confirm if the logic checks out: If we asynchronously call Cancel() from the DataStreamSender, before this function has actually started executing, we would be hitting DCHECKs in RpcController::Cancel(). Is there a way we are going to make sure that doesn't happen, since the RPC layer doesn't synchronize between this and Cancel() itself? -- To view, visit http://gerrit.cloudera.org:8080/7743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 2: I think you may need to update test admission-reject-min-reservation.test now too, since that went in last night -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Tim Armstrong has uploaded a new patch set (#2). Change subject: IMPALA-5855: reserve enough memory for preaggs .. IMPALA-5855: reserve enough memory for preaggs The calculation in the planner failed to account for the behaviour of Suballocator, which needs to obtain at least one buffer to allocate any memory. Testing: Added a regression test that caused a crash before the fix. Updated planner tests. Was able to run local stress test binary search to completion (it previously crashed). Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b --- M fe/src/main/java/org/apache/impala/planner/AggregationNode.java M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test M testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test M testdata/workloads/functional-query/queries/QueryTest/aggregation.test 7 files changed, 85 insertions(+), 71 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/7871/2 -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-5830: SET DENY RESERVATION PROBABILITY test
Michael Brown has posted comments on this change. Change subject: IMPALA-5830: SET_DENY_RESERVATION_PROBABILITY test .. Patch Set 2: (3 comments) Mostly naming/commenting nits. http://gerrit.cloudera.org:8080/#/c/7809/2/tests/query_test/test_spilling.py File tests/query_test/test_spilling.py: PS2, Line 33: TestSpilling Does it make sense to call this TestSpillingDebugActionDimensions ? Or, consider adding a comment here that's similar to L72. PS2, Line 71: TestSpillingNoDebugAction Maybe append Dimensions to this name? At least one test uses debug actions, just using query options. PS2, Line 95: setting debug_action to alternative values Maybe append the comment "via query options". -- To view, visit http://gerrit.cloudera.org:8080/7809 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida6b55b2dee0779b1739af5d75943518ec40d6ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
