Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8621 )
Change subject: IMPALA-3703: Store query context in thread-local variables ...................................................................... Patch Set 1: (2 comments) On the JIRA, Dan asked about this being a basis for things like prepending log messages with query ids and so on. The unstructured element of just appending to a 4k block of memory may make that hard. Did you consider doing a more structured system? (e.g., exposing setQueryId() and setThreadName() and having those store values or offsets/lengths into the buffer.) There are trade-offs of course. Relatedly, how hard will it be to visit a /threads debug page and see what all the threads are up to? Have you tried pulling one of these out in a minidump? The commit message mentioned gdb, but minidumps have their own tooling. For what it's worth, if we wanted to avoid the stack allocation, it looks like the following API exists: // Register a block of memory of length bytes starting at address ptr // to be copied to the minidump when a crash happens. void RegisterAppMemory(void* ptr, size_t length); I'd encourage you to write a test. There's a little bit of byte-counting in making sure we don't overflow the array. I think it's right, but should be testable. http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h File be/src/common/thread-info.h: http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@70 PS1, Line 70: /// Call this global function to add information about the current thread Should we document that you should avoid information that's sensitive here? i.e., we should avoid things like query text because we're trying to avoid them in minidumps? Or not? http://gerrit.cloudera.org:8080/#/c/8621/1/be/src/common/thread-info.h@73 PS1, Line 73: /// is a no-op. It's weird to me that this can be a no-op if there's no object, but it fails at line 48 if it's full. -- To view, visit http://gerrit.cloudera.org:8080/8621 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609 Gerrit-Change-Number: 8621 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 21 Nov 2017 22:55:23 +0000 Gerrit-HasComments: Yes
