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 8: (8 comments) Looks pretty good mainly had comments about comments and tests http://gerrit.cloudera.org:8080/#/c/14143/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14143/8//COMMIT_MSG@27 PS8, Line 27: Added some test coverage for coordinator side code paths for writing Thank you for addressing the test gap! http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@91 PS8, Line 91: void Wait(); Mention that this log query events. Also, can you mention that this is called only by wait_thread_ and that any other threads that want to wait for these conditions need to call BlockOnWait()? I think that relationship is a little unclear without reading the code. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@96 PS8, Line 96: /// BlockOnWait() may be called after WaitAsync() has been called in order to wait This comment needs to be updated, since we're waiting for wait_thread_ to signal the condition variable. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@333 PS8, Line 333: singal typo: signal http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@335 PS8, Line 335: ConditionVariable block_on_wait_cv_; We could probably use a CountingBarrier instead of this pair of variables and slightly reduce the amount of code. I think the current approach is perfectly fine though. http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.cc@811 PS8, Line 811: block_on_wait_cv_.NotifyAll(); nit: the best practice is to signal the CV after dropping the log (otherwise signalled threads might wake up, then immediately block on acquiring the lock). http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/impala-server.cc@a642 PS8, Line 642: I ended up pulling out the before and after code into before.cc and after.cc to review the moved code. Confirmed the changes were minimal and made sense. http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test File testdata/workloads/functional-query/queries/QueryTest/lineage.test: http://gerrit.cloudera.org:8080/#/c/14143/8/testdata/workloads/functional-query/queries/QueryTest/lineage.test@268 PS8, Line 268: test_haha_e4f3f13c Do we know that these unique database names are stable across different systems, etc? -- 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: 8 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: radford nguyen <radford.ngu...@gmail.com> Gerrit-Comment-Date: Sat, 07 Sep 2019 00:17:55 +0000 Gerrit-HasComments: Yes
