Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8303 )
Change subject: IMPALA-1575: Part 1: eagerly release query exec resources ...................................................................... Patch Set 11: Code-Review+1 (4 comments) Thanks for the comments - I had to think hard about each one of them. http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc@1095 PS10, Line 1095: query_state_ != nullptr) > How would query_state_ be NULL if the coordinator hasn't release its refere Your reasoning appears to be correct. My tendency is to keep the null check though - usually unnecessary checks like this just add confusion but on cleanup logic I find it's a good practice to write defensive code like this since code tends to change, it's hard to reason about the different possible failure modes and test coverage isn't always complete. I'm open to removing it though, I think it would need a comment to explain why it's valid though. http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc@322 PS10, Line 322: while (tracker != nullptr && !tracker->is_query_mem_tracker_) { : tracker = tracker->parent_; : } > What's the thread safety story here? What if a any one of the trackers touc The parent_ pointer is never modified so the only race possible is if intervening MemTrackers are destroyed. This is not possible since the MemTracker hierarchy is now all torn down at once when the query ends. The thing about the parent pointer isn't documented anywhere so I made the pointer const and added a comment. http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@96 PS10, Line 96: if (query_mem_tracker_ != nullptr) { : // Disconnect the query MemTracker hierarchy from the global hierarchy. After this : // point nothing must touch this query's MemTracker and all tracked memory associated : // with the query must be released. The whole query subtree of MemTrackers can : // therefore be safely destroyed. : > I'm still not clear why this moved from ReleaseExecResources() to ~QuerySta It happens at the same time as before - previously ReleaseResources() ran immediately before the destructor. After this patch everything in ReleaseExecResources() can happen much sooner before the destructor. However this one action of disconnecting the query MemTracker couldn't be moved earlier. I tried to improve the comment a bit to emphasise that this was disconnecting a control structure. I also thought about making a separate Close() method that did this kind of teardown, but it seemed unnecessary given that it would run immediately before the destructor. I.e. query_state->Close(); delete query_state http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@135 PS10, Line 135: initial_reservations_->Init(query_id(), rpc_params.min_reservation_bytes)); > Can we move this to the start of Init() and make L95 a DCHECK? I like the idea but after trying it out I think it's significantly complicated by the fact that the coordinator grabs a resource refcount without calling Init() - in some cases when Init() fails the refcount is already non-zero so it wouldn't be valid to release the resources yet. -- To view, visit http://gerrit.cloudera.org:8080/8303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41ff374b0403f10a145f7fee9b3145953ee32341 Gerrit-Change-Number: 8303 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 26 Oct 2017 21:59:07 +0000 Gerrit-HasComments: Yes
