Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14975 )
Change subject: IMPALA-9154: Make runtime filter propagation asynchronous ...................................................................... Patch Set 1: (9 comments) You've got several formatting issues. Instead of me pointing all of them out, please run clang-format http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-backend-state.cc@573 PS1, Line 573: if (it == filter_routing_table.get()->id_to_filter.end()) { This should be possible since filter_routing_table_ will be initialized with all filter ids, right? I think you can leave off the 'if' and just have a DCHECK http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-filter-state.h@91 PS1, Line 91: int* Its bad encapsulation to return a pointer to a class variable like this. Better to use some sort of setter function. Maybe something like "IncrementNumInflightRpcs()" which you pass either 1 or -1 into as appropriate, or possibly "set_num_inflight_rpcs()" http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-filter-state.h@100 PS1, Line 100: Disable I think it would be more natural to call this one DisableAndRelease() and the other one just Disable(). http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator-filter-state.h@142 PS1, Line 142: Initialized to -1 to indicate that the associated : /// runtime filter has never been involved in the propagation. I don't think that you rely on this being the case anywhere? Should be fine to just initialize it to 0, which will make the logic around incrementing it simpler. http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.h@41 PS1, Line 41: #include <condition_variable> move this up with the other "#include <...>" in the first group http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.cc@1144 PS1, Line 1144: lock_guard<SpinLock> l(filter_routing_table_->update_lock); So I think we'll need to think about the locking protocol here a little more. 'update_lock' is a single global lock for all filters in a query, and this patch increases contention on it because it 1) increases the size of the critical section here by holding it while all of the calls to BackendState::PublishFilter() execute 2) is taken in PublishFilterCompleteCb() once for each filter for each backend that receives it. I think that instead, we should make per-filter locks, eg. add a 'lock_' to FilterState (there's a TODO in coordinator-filter-state.h for this). That lock would then be used to protect access to pending_count_, num_inflight_rpcs_, etc. So, you can remove this line and take FilterState::lock_ after we get the FilterState from the map on line 1156 or so. Accessing the map should be fine because we've already taken filter_routing_table_->lock above, and filter_routing_table_ itself can't get concurrently modified with UpdateFilter() since its setup in InitFilterRoutingTable(), which can't run concurrently with UpdateFilter(), and then the map itself never changes. One thing that's maybe a little tricky is how to rewrite your hasInflightKRPCsForFilterPropagation() logic to deal with this. The way you're doing it now - where we have a single condition variable that gets signaled each time a filter is done and then you check to see if all filters are done and wait again if not, is a little unusual. My suggestion would be to also make it per-filter, eg. add a FilterState::update_filters_done_cv_ and then a FilterState::WaitForPublishFilter() and then in ReleaseExecResources() you can just iterate over all the FilterStates and call WaitForPublishFilter() on them. You'll of course want to check all of my reasoning here. http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.cc@1217 PS1, Line 1217: !IsExecuting() && rpc_params.has_bloom_filter() I think this is wrong - as written, if !IsExecuting() is true but its a min-max filter, we won't return and will still try to publish the filter. http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/coordinator.cc@1306 PS1, Line 1306: if (is_bloom_filter()) { You can save some code duplication by calling DisableButNotRelease() here first and then doing the release. http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/14975/1/be/src/runtime/runtime-filter-bank.cc@197 PS1, Line 197: ? -- To view, visit http://gerrit.cloudera.org:8080/14975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb6726d349be701f3a0602b2ad5a934082f188a0 Gerrit-Change-Number: 14975 Gerrit-PatchSet: 1 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 08 Jan 2020 21:44:36 +0000 Gerrit-HasComments: Yes
