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 2: Code-Review+1 (2 comments) This looks good to me. Thanks for adding the tests. The Thrift comments below are largely philosophical, though they wouldn't be in the case of the query profile. 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 user > Done. Does this mean ideally we shouldn't change those numbers or removing In the strictest sense, you'd never re-use the numbers. You can remove an existing field today, just don't re-use the number. See https://www.quora.com/What-are-the-key-differences-between-Apache-Thrift-Google-Protocol-Buffers-MessagePack-ASN-1-BERT-and-Apache-Avro-All-of-these-provide-binary-serialization-RPC-frameworks-and-IDL-What-are-their-different-characteristics-performance-etc/answer/Philip-Zeyliger for more on this stuff, but we're not as affected by it here. (In something like the Impala Runtime Profile, we are affected by this: we serialize that to log files which tools read, and it'd be bad to break these rules there.) http://gerrit.cloudera.org:8080/#/c/8589/2/common/thrift/LineageGraph.thrift File common/thrift/LineageGraph.thrift: http://gerrit.cloudera.org:8080/#/c/8589/2/common/thrift/LineageGraph.thrift@69 PS2, Line 69: 8: required Types.TUniqueId query_id It doesn't actually matter because we don't serialize these, but 'required' typically means that this will fail to parse an old serialized TLineageGraph from previous versions of Impala. I think your usage here is consistent with how we use Thrift here, so I think it's likely harmless. -- 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: 2 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Comment-Date: Wed, 22 Nov 2017 00:11:21 +0000 Gerrit-HasComments: Yes
