Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14143 )
Change subject: IMPALA-8572: Log query events before unregister. ...................................................................... Patch Set 9: (7 comments) 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: /// Since this is a blocking operation, it is invoked from a separate thread > Mention that this log query events. Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@96 PS8, Line 96: /// on Wait() are signalled *before* logging the events so that they can resume their > This comment needs to be updated, since we're waiting for wait_thread_ to s Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@333 PS8, Line 333: > typo: signal Done http://gerrit.cloudera.org:8080/#/c/14143/8/be/src/service/client-request-state.h@335 PS8, Line 335: /// TODO: remove and use ExecEnv::GetInstance() instead > We could probably use a CountingBarrier instead of this pair of variables a Personally I feel like this is more readable but let me know if you feel strongly about using a barrier, I can switch. 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: } > nit: the best practice is to signal the CV after dropping the log (otherwis Nice catch, done. 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.c Thanks. 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: > Do we know that these unique database names are stable across different sys Yes, this is the reason the precommit failed. Fixed it. -- 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: 9 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 21:13:09 +0000 Gerrit-HasComments: Yes
