impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) PREVIEW: IMPALA-2550 Introduce query-wide execution context.
Date Thu, 21 Jul 2016 17:53:25 GMT
Henry Robinson has posted comments on this change.

Change subject: PREVIEW: IMPALA-2550 Introduce query-wide execution context.

Patch Set 2:

(1 comment)
File be/src/service/fragment-mgr.h:

PS2, Line 80: std::shared_ptr<
> whoever incremented it in the first place, when it's done with the object.
It sounds like you're describing basically shared_ptr acquire / release semantics, but with
explicit 'release' that must be called by the caller.

That sounds error prone, and a pattern that we avoid in plenty of other areas: lock_guard,
NotifyBarrierOnExit, ClientConnection<T>, ScopedSessionState, just to mention four.
Mandatory 'clean up' methods tied to object lifetime are easy to miss, and often hard to debug.

Why not use shared_ptr, but ensure that the destructor does no work? I.e.: there's still an
explicit reference count, which is managed by the fragments only. When that hits zero, the
QueryExecState is removed from query_exec_state_map_, and if necessary a CleanUp() method
can be called to tear down any 'live' state (like stopping a reporting thread, etc). 

But other clients can hold onto a shared_ptr, which ensures the object itself remains valid.
When the last client destroys its shared_ptr, the object is destroyed, but there's no real
work to be done beyond releasing the memory it uses so there's less danger of bugs due to
nondeterministic destructor execution.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c513d08f718699ba0c4cdb90c117aaecf95d7fc
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message