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 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/11005/6/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/11005/6/be/src/runtime/coordinator.h@301 PS6, Line 301: /// Synchronizes updates to the filter_routing_table_. : SpinLock filter_update_lock_; : : /// Protects filter_routing_table_. : /// Usage pattern: : /// 1. To update filter_routing_table_: Acquire shared access on filter_lock_ and : /// upgrade to exclusive access by subsequently acquiring filter_update_lock_. : /// 2. To read, initialize/destroy filter_routing_table: Directly acquire exclusive : /// access on filter_lock_. : boost::shared_mutex filter_lock_; : > maybe write a consolidated comment to better explain the need for two locks Done http://gerrit.cloudera.org:8080/#/c/11005/6/be/src/runtime/coordinator.h@334 PS6, Line 334: : /// Called when the query is done executing due to reaching EOS or client : /// cancellation. If 'exec_state_' != EXECUTING, does nothing. > nit: "Caller must have exclusive access to filter_lock_" Done http://gerrit.cloudera.org:8080/#/c/11005/6/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/11005/6/be/src/runtime/coordinator.cc@265 PS6, Line 265: lock_guard<shared_mutex> lock(filter_lock_); : for (const FragmentExecParams& fragment_params: schedule_.fragment_exec_params()) { : int num_instances = fragment_params.instance_exec_par > fyi: this method is called in exec before any fragments are started. Also, Make sense. I have removed the comment. I have kept the lock_guard for the sake of keeping a consistent code flow across all accesses. Acquiring the lock should not be expensive if there is no contention on it. Or would you recommend entirely removing it form here? -- 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: 7 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, 03 Aug 2018 20:24:15 +0000 Gerrit-HasComments: Yes
