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 4: (10 comments) Patch is looking very good, most of my feedback this time is related to your comments. At a high level - remember to try to keep comments concise, and to approach them not from a perspective of "why I did it this way for this specific patch" but from a perspective of "what would someone coming along later reading this code need to know to understand how it works" http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-backend-state.cc@582 PS4, Line 582: e.g., request dropped due to : // backpressure, unnecessary http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-backend-state.cc@596 PS4, Line 596: // Recall that the thread that called Coordinator::BackendState::PublishFilter() : // associated with this FilterState acquired the corresponding FilterState::lock_ : // while issuing the PublishFilter()'s related to this FilterState. Thus, when : // 'num_flight_rpcs_' of this FilterState reaches 0, this FilterState should : // already been disabled, i.e., no more PublishFilter() related to this FilterState : // will be issued. This can be made more concise, eg: Since we disable the filter once complete and hold FilterState::lock_ while issuing all PublishFilter() rpcs, at this point there can't be any more PublishFilter() rpcs issued. http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h@108 PS4, Line 108: since the memory could : /// still be referenced by some inflight KRPC's. : /// Refer to Coordinator::UpdateFilter() for further detail. Unnecessary, can be removed http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h@111 PS4, Line 111: DisableButNotRelease I think its fine to just call this Disable() http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h@118 PS4, Line 118: /// Before calling ReleaseExecResources() to disable all the associated runtime filters : /// and release the consumed memory, we need to make sure there is no inflight KRPC : /// propagating the aggregated filter. This comment is a little weird, cause its focused on how the function is used at one particular call site (which yes, happens to be the only call site for now, but it might not always be). Let's rewrite it to just be about what the function actually does, eg: Waits until any inflight PublishFilter rpcs have completed. http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h@159 PS4, Line 159: KRPC's involved in the propagation of the : /// associated runtime filter. This is a little vague, since there are multiple runtime filter related rpcs. Lets change it to something like "PublishFilter rpcs" http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator-filter-state.h@161 PS4, Line 161: num_inflight_rpcs_ Similarly, I know its verbose, but probably better to call this 'num_inflight_publish_filter_rpcs_' http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator.cc@1054 PS4, Line 1054: l.release(); Shouldn't be needed - unique_lock will automatically release the lock when it goes out of scope (which happens at the end of each loop iteration) http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator.cc@1139 PS4, Line 1139: DCHECK(it != filter_routing_table_->id_to_filter.end()); So I know that in a previous iteration of this patch, I told you to change a similar place from an 'if' to a DCHECK, but I think this case is a little different than that one. The reason is that this is directly taking input from an rpc without any previous checking for the validity of that input. Of course, its extremely unlikely that something would happen like the rpc parameters getting corrupted without the rpc layer noticing, but its still the sort of situation where its better to be defensive, eg. in case there's a bug in Impala where we get something about how we generate/send the rpcs wrong, esp. because the DCHECK won't be there in production deployments so its pretty unclear what would even happen if the input was wrong, and it could potentially be something worse than just crashing the impalad, like corrupting on-disk data. By contrast, if you look at the place where I made the comment in the earlier version of the patch, it was in BackendState::PublishFilter, which logically comes after this 'if' and therefore its basically impossible to imagine a DCHECK for this there ever being hit. http://gerrit.cloudera.org:8080/#/c/14975/4/be/src/runtime/coordinator.cc@1189 PS4, Line 1189: // Note that UpdteFilter() could still be invoked even when 'pending_count_' reaches : // 0. For instance, when the corresponding join is a broadcast join where : // 'pending_count_' was initialized to 1 since each local filter is the same. : // In this regard, we need to disable 'state' to prevent another KRPC from calling : // ApplyUpdate() again to update the aggregated filter since this would be a : // redundant operation. This can be made more concise, eg: Filter is complete, so we disable it so future UpdateFilter rpcs are ignored (eg. if it was a broadcast join). -- 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: 4 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: Tue, 14 Jan 2020 19:13:04 +0000 Gerrit-HasComments: Yes
