Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 )
Change subject: IMPALA-8572: Log query events before unregister. ...................................................................... Patch Set 10: (5 comments) Left a few more comments while trying to understand the test failure. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@749 PS10, Line 749: wait_thread_->Join(); Should we drop the lock while joining on the wait thread? I feel like there's some risk of introducing a deadlock if Wait() ever acquires the lock. I think this should be safe since no other thread should be concurrently modifyin wait_thread_. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@754 PS10, Line 754: LogQueryEvents(); We could probably also drop the lock when calling this. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@780 PS10, Line 780: if (wait_thread_.get() == nullptr || is_wait_done_) return; I think we can actually just delete this line, the next line is sufficient. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1395 PS10, Line 1395: Status status = query_status(); Accessing query_status without acquiring a lock might not actually be safe now - I think it previous used to work because this code ran *after* the query status could no longer change. http://gerrit.cloudera.org:8080/#/c/14143/10/be/src/service/client-request-state.cc@1426 PS10, Line 1426: const TExecRequest request = exec_request(); Should be a const TExecRequest& to avoid copying. -- To view, visit http://gerrit.cloudera.org:8080/14143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I639b9c1acb9806b29292cd85be2863688453ca2e Gerrit-Change-Number: 14143 Gerrit-PatchSet: 10 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: radford nguyen <[email protected]> Gerrit-Comment-Date: Tue, 10 Sep 2019 23:01:56 +0000 Gerrit-HasComments: Yes
