impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6210: Add query id to lineage graph logging
Date Wed, 22 Nov 2017 00:11:21 GMT
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 <twang@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Nov 2017 00:11:21 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message