Henry Robinson has posted comments on this change. Change subject: IMPALA-3873: Add QueryStateAccessor ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/impala-server.h File be/src/service/impala-server.h: PS1, Line 537: class QueryRecordAccessor; : class QueryExecStateAccessor; > Are these two forward declarations needed? Yes - you have to declare a nested class in its parent's declaration, otherwise the definition will not compile. Line 544: bool GetQueryStateAccessor(const TUniqueId& query_id, > Why not return the unique_ptr and return an empty one in case the query can Done http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.cc File be/src/service/query-state-accessor.cc: Line 156: bool ImpalaServer::GetQueryStateAccessor(const TUniqueId& query_id, > This implementation was hard to find. Maybe add a comment in impala-server. Done http://gerrit.cloudera.org:8080/#/c/3851/1/be/src/service/query-state-accessor.h File be/src/service/query-state-accessor.h: PS1, Line 32: Clients must lock() and unlock() > Would it be possible to lock the wrapped objects upon creation and unlock w This is an interesting idea, but I'd prefer not to. This interface makes it explicit that there is a lock that is taken, and that doing so means that access to the underlying state may be excluding some other reader. Hiding that in the constructor means that clients could unwittingly hold onto the reference for much longer than they would otherwise intend to. -- To view, visit http://gerrit.cloudera.org:8080/3851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec3dae66a81988c99cde1516ff511186e17dd8c0 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
