Lars Volker 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? Line 544: bool GetQueryStateAccessor(const TUniqueId& query_id, Why not return the unique_ptr and return an empty one in case the query cannot be found? 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.h where to find it? 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 when QueryStateAccessor goes out of scope itself? The places where this change uses it look like the lock_guard always has the same lifetime as the QueryStateAccessor itself. -- 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: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
