Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14677 )
Change subject: IMPALA-9137, IMPALA-9138: Mark failed RPCs as retryable and update blacklist ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Common.thrift File common/thrift/Common.thrift: http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Common.thrift@1 PS3, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > Not really sure I see any benefit - I guess it very slightly reduces recomp My understanding of Types.thrift (and I could be wrong here) is that it it is meant to store Impala types (e.g. TPrimitiveType, TScalarType, etc.). However, it looks like a lot of unrelated stuff crept in over time, and I can't really figure out what Types.thrift is suppose to encapsulate. Common.thrift is suppose to represent just commonly useful Thrift structs (e.g. TNetworkAddress) that are used across Thrift files. TUniqueId could probably live in here as well. I guess the re-factoring is an attempt to stop polluting Types.thrift and start cleaning it up. This isn't really part of the core patch, I just thought it was nice to have. So I don't have strong opinions about keeping this in, but just wanted to explain my thought process. http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Status.thrift File common/thrift/Status.thrift: http://gerrit.cloudera.org:8080/#/c/14677/3/common/thrift/Status.thrift@48 PS3, Line 48: 4: optional TRPCErrorMessage rpc_msg > Agreed that a string->string map was a bad suggestion, just throwing stuff yeah, the point about constructing new Status objects from existing ones is a valid concern. I'll try to think through this some more. Curious if @Michael Ho has any thoughts. -- To view, visit http://gerrit.cloudera.org:8080/14677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I733cca13847fde43c8ea2ae574d3ae04bd06419c Gerrit-Change-Number: 14677 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 26 Nov 2019 01:14:41 +0000 Gerrit-HasComments: Yes
