Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8971 )
Change subject: IMPALA-5519: Allocate Runtime filter memory from Buffer pool ...................................................................... Patch Set 2: (21 comments) http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator-backend-state.cc@410 PS2, Line 410: return res.__isset.status ? Status(res.status) : Status::OK(); > Why not always set TPublishFilterParams.status. This seems to be adding ano Removed http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator.cc@1179 PS2, Line 1179: // Cancel query if the result of PublishFilter rpc returns a non-ok status. > Nit: slightly redundant wording with result/returns. E.g. "if the PublishFi As discussed, removing cancellation logic http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator.cc@1182 PS2, Line 1182: "This Error was encountered in Fragment $0 at $1:$2", fragment_idx, > nit: don't need caps on Error/Fragment. As discussed, removing cancellation logic http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/coordinator.cc@1184 PS2, Line 1184: Cancel(&status); > If we're going to cancel the query there's no point sending out more filter As discussed, removing cancellation logic http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.h@111 PS2, Line 111: input > output? Done http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.h@118 PS2, Line 118: *& > Let's just use a double pointer for the output to be consistent with the re Done http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.h@165 PS2, Line 165: total_filter_mem_required_ > I think total_bloom_filter_mem_required_ would be more consistent with the Done http://gerrit.cloudera.org:8080/#/c/8971/1/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/8971/1/be/src/runtime/runtime-filter-bank.cc@189 PS1, Line 189: if (buffer_pool_client_.GetUnusedReservation() < required_space) { > I don't see this change in PS2 my mistake, I only added the DCHECK in AllocateScratchBloomFilter, forgot to put it here too. Will add it in the next patch. http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.cc@a78 PS2, Line 78: > Hmm so we were unnecessarily re-allocating filters if they were already reg yes thats right http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/runtime/runtime-filter-bank.cc@a80 PS2, Line 80: > Was the lock removed accidentally? yes, I will add it back http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.h@71 PS2, Line 71: void Close(); > Maybe mention that Close() is safe to call multiple times or if Init() wasn Done http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.h@111 PS2, Line 111: /// buffer pool allocation failed and the filter should be discarded. > Is the change in this function's behaviour needed now? I would prefer to keep this change in case this methods is called before calling Init() or after calling close. I will update the comment to more appropriately reflect the recent change. http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.cc@30 PS2, Line 30: : always_false_(true), > Maybe initialize the constant ones at their definition while we're here. I Done http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.cc@54 PS2, Line 54: RETURN_IF_ERROR( : buffer_pool_->AllocateBuffer(buffer_pool_client_, alloc_size, &buffer_handle_)); will call Close() before this to make sure Init() is safe to call multiple times. http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.cc@63 PS2, Line 63: directory_ > directory_ != nullptr, to be consistent with the rest of the code Done http://gerrit.cloudera.org:8080/#/c/8971/2/be/src/util/bloom-filter.cc@72 PS2, Line 72: if (directory_) { > directory_ != nullptr, to be consistent with the rest of the code Done http://gerrit.cloudera.org:8080/#/c/8971/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/8971/2/common/thrift/ImpalaInternalService.thrift@833 PS2, Line 833: 1: optional Status.TStatus status > Maybe it should be required? Given that we don't support daemons of differe As discussed, removing cancellation logic http://gerrit.cloudera.org:8080/#/c/8971/2/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/8971/2/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@120 PS2, Line 120: private long runtimeFiltersMemRequirement_ = 0; > Maybe runtimeFiltersReservation_? Done http://gerrit.cloudera.org:8080/#/c/8971/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/8971/2/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@110 PS2, Line 110: Make > nit: extraneous caps on make Done http://gerrit.cloudera.org:8080/#/c/8971/2/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test File testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test: http://gerrit.cloudera.org:8080/#/c/8971/2/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test@391 PS2, Line 391: # mem requirement after accounting for the memory > It seems like the new test case won't exercise the code path on the coordin the old test was already flawed in that it never exercised the code path that it describes. The code path it describes should be this https://github.com/apache/impala/blob/b27537a15b679c0c84b7d2e9f49c9a51f9ae93ba/be/src/runtime/coordinator.cc#L1198 The reason it worked was that an always true filter was created when mem allocation in one of the fragments failed due to low mem limit being set. As a result the code path being hit was this: https://github.com/apache/impala/blob/b27537a15b679c0c84b7d2e9f49c9a51f9ae93ba/be/src/runtime/coordinator.cc#L1195 Moreover the mem_tracker in coordinator only allocates memory once for the thrift filter object inside Coordinator::FilterState, ans thereafter the new filters recieved are only ORed and their memory(already allocated by thrift) is not accounted for by the coordinator. In order for this to work exactly according to the description, we would have to come up with a query that has a mem_limit which would allow filter creation in the fragment instance but disallow even one filter creation in the coordinator. Also, after the changes in this patch, filter memory will always be accounted for in the plan, and the admission controller will never let the user put a mem limit lower than required to allocate a filter. TLDR: its ok to remove the old test case as it wasn't testing what it was supposed to. http://gerrit.cloudera.org:8080/#/c/8971/2/testdata/workloads/functional-query/queries/QueryTest/runtime_row_filters.test@416 PS2, Line 416: prefectly > nit: perfectly Done -- To view, visit http://gerrit.cloudera.org:8080/8971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea2759665fb2e8bef9433014a8d42a7ebf99ce1f Gerrit-Change-Number: 8971 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 17 Jan 2018 18:59:17 +0000 Gerrit-HasComments: Yes
