Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12129 )
Change subject: IMPALA-6664: Tag log statements with fragment or query ids. ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/logging.cc File be/src/common/logging.cc: http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/logging.cc@55 PS2, Line 55: kZeroUniqueId ZERO_UNIQUE_ID (This is a difference from the google C++ style for better or worse: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536) http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/logging.cc@63 PS2, Line 63: if (id == kZeroUniqueId) { nit: one line http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/logging.cc@67 PS2, Line 67: if (changed != nullptr) { nit: one line http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/thread-debug-info.h File be/src/common/thread-debug-info.h: http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/thread-debug-info.h@a116 PS2, Line 116: I think there was some reason this was a human-readable string - maybe to make it more readable if we printed in the minidump handler? Zoltan probably remembers. Anyway, this seems OK too. http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/common/thread-debug-info.h@118 PS2, Line 118: TUniqueId query_id_; Can you document that these are 0 if this thread isn't part of a query or instanc? http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/12129/2/be/src/runtime/io/request-context.h@403 PS2, Line 403: TUniqueId instance_id_; Can you document that these are 0 if we're not working on behalf of a query? http://gerrit.cloudera.org:8080/#/c/12129/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/12129/2/tests/common/impala_test_suite.py@843 PS2, Line 843: def assert_impalad_log_contains(self, level, line_regex, expected_count=1): I don't see anything using this. -- To view, visit http://gerrit.cloudera.org:8080/12129 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6634ef9d1a7346339f24f2d40a7a3aa36a535da8 Gerrit-Change-Number: 12129 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Tue, 08 Jan 2019 19:34:01 +0000 Gerrit-HasComments: Yes
