Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8589 )
Change subject: IMPALA-6210: Add query id to lineage graph logging ...................................................................... Patch Set 1: (4 comments) Thanks! This looks largely good to me. Would you mind adding an assertion to tests/custom_cluster/test_lineage.py that the query id is populated, and is maybe the same as reported in the profile? It's trivial, but it'd be nice to convince ourselves that TUniqueIdUtil.java matches the C++ implementations, too. http://gerrit.cloudera.org:8080/#/c/8589/1/common/thrift/LineageGraph.thrift File common/thrift/LineageGraph.thrift: http://gerrit.cloudera.org:8080/#/c/8589/1/common/thrift/LineageGraph.thrift@55 PS1, Line 55: 3: required string hash It's poor form to renumber thrift structs. Typically, you just use the next available number. This makes serialized structs compatible over time. (In practice, these are internal RPCs and are unlikely to have been serialized, but I think it's best not to violate the rules.) See https://developers.google.com/protocol-buffers/docs/proto#assigning-tags about "unique numbered tags". Thrift and ProtoBuf are basically isomorphic in this respect. http://gerrit.cloudera.org:8080/#/c/8589/1/fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java File fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java: http://gerrit.cloudera.org:8080/#/c/8589/1/fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java@25 PS1, Line 25: public class TUniqueIdUtil { I didn't see any test code for this. It'd be good to unit test it, and confirm that you're getting the same results as the tests in be/src/util/debug-util-test.cc. Should be quick, and it'd be good to check that the Java and C++ versions of this code are identical. http://gerrit.cloudera.org:8080/#/c/8589/1/fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java@26 PS1, Line 26: public static String Print(TUniqueId id) { This is called PrintId() and ParseId() in be/src/util/debug-util.cc. Perhaps use the same name to make it more obvious? http://gerrit.cloudera.org:8080/#/c/8589/1/fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java@31 PS1, Line 31: String[] splitted = id.split(":"); nit: one space after =. -- To view, visit http://gerrit.cloudera.org:8080/8589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4adbd02df37a234dbb79f58b7c46ca11a914229f Gerrit-Change-Number: 8589 Gerrit-PatchSet: 1 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Nov 2017 23:25:18 +0000 Gerrit-HasComments: Yes
