impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1575: Part 1: eagerly release query exec resources
Date Thu, 26 Oct 2017 21:59:07 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 21:59:07 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message