Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15822 )
Change subject: IMPALA-9692 (part 1): Refactor TBackendDescriptor to protobuf ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h File be/src/util/container-util.h: http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@66 PS1, Line 66: STATIC_ASSERT_SIZE(UniqueIdPB, 40); > should we add != and < overloads as well? while they might not be used now, Done http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@70 PS1, Line 70: } > why not use std::tie like TUniqueId does? std::tie operates on lvalues, which works for Thrift since it lets you directly access the class variables, but for protobuf since everything is done with getter functions we only have rvalues. http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@88 PS1, Line 88: return !(lhs == rhs); > same as above, why not use std::tie Done http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@151 PS1, Line 151: } : }; : : } // end namespace std : : namespace impala { : : struct HashTNetworkAddressPtr : public std::unary_function<TNetworkAddress*, size_t> { : size_t operator()(const TNetworkAddress* const& p) const { return hash_value(*p); } : }; > these aren't use anywhere, right? grepped the be/ code and didn't see any r Yeah, agreed these aren't used anywhere. http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/network-util.h File be/src/util/network-util.h: http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/network-util.h@63 PS1, Line 63: /// If the given string address is malformed, returns a network address with an empty > ahhh should we move it so its come right after "TNetworkAddress MakeNetwork Done -- To view, visit http://gerrit.cloudera.org:8080/15822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7d1e373d9c87887144517ff6a4c2d5996aa88b8 Gerrit-Change-Number: 15822 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 05 May 2020 01:08:40 +0000 Gerrit-HasComments: Yes
