Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11005 )
Change subject: [WIP] IMPALA-6153: Execute UpdateFilter() only for executing queries ...................................................................... Patch Set 4: (7 comments) I have modified the use of the filter_lock_. While addressing comments by Bikram and Sailesh, I noticed that the `FilterDebugString()` function has two invocations (called in InitFilterRoutingTable() and ReleaseExecResources()). Acquiring the filter_lock_ in FilterDebugString() was incorrect because the lock was already acquired by the caller in some cases (ReleaseExecResources()). On the other hand, not acquiring the filter_lock_ in the InitFilterRoutingTable() breaks the control flow that a thread always acquires filter_lock_ before altering the filter_routing_table_. So I modified InitFilterRoutingTable to reflect this logic. @Bikram @Sailesh, could you please review this part of the code and verify that the comments make sense? Thanks, Pooja http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.h@302 PS4, Line 302: it > nit: this lock Done http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.h@309 PS4, Line 309: releases the memory associated with : /// filter_routing_table_ > nit: alters the filter_routing_table_ Done http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.h@311 PS4, Line 311: boost::shared_mutex > We have to ensure that this is a 'writer preferred' shared mutex. It turns Ack http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc@79 PS4, Line 79: exec_state_.Load(), ExecState::EXECUTING > not your change, but replace with: Done http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc@397 PS4, Line 397: lock_guard<SpinLock> l(filter_update_lock_); > Why not just get exclusive access to 'filter_lock_' here? I have actually removed this line from the function. http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc@454 PS4, Line 454: exec_state_.Load() != ExecState::EXECUTING > not your change, but replace with: Done http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc@714 PS4, Line 714: exec_state_.Load() == ExecState::EXECUTING > not your change, but replace with: Done -- To view, visit http://gerrit.cloudera.org:8080/11005 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I669db217f86db5ff2802335e7b1ae8027ea7161c Gerrit-Change-Number: 11005 Gerrit-PatchSet: 4 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 30 Jul 2018 19:30:36 +0000 Gerrit-HasComments: Yes
