Sailesh Mukil 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: (8 comments) 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 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_ 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 out that boost doesn't have an option to specify a preference, and uses a "fair" algorithm instead to decide which waiter to schedule next, so I guess this is fine. 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: !IsExecuting() 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? 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: !IsExecuting() 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: IsExecuting() http://gerrit.cloudera.org:8080/#/c/11005/4/be/src/runtime/coordinator.cc@871 PS4, Line 871: if (!IsExecuting()) return; Instead of doing this here, I would say you can replace the IsDone() check inside CoordinatorBackendState::PublishFilter() with this IsExecuting() check. CoordinatorBackendState has a reverse reference to the Coordinator object. -- 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 <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 27 Jul 2018 21:17:37 +0000 Gerrit-HasComments: Yes
