Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15822 )
Change subject: IMPALA-9692 (part 1): Refactor TBackendDescriptor to protobuf ...................................................................... Patch Set 1: (6 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: // UniqueIdPB should we add != and < overloads as well? while they might not be used now, the implementations should be pretty straightforward. concerned that other devs touching this code might assume these overloads exist and get bitten when they eventually realize they aren't. http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@70 PS1, Line 70: return lhs.hi() == rhs.hi() && lhs.lo() == rhs.lo(); why not use std::tie like TUniqueId does? http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@88 PS1, Line 88: return lhs.hostname() == rhs.hostname() && lhs.port() == rhs.port(); same as above, why not use std::tie http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/container-util.h@151 PS1, Line 151: struct HashTNetworkAddressPtr : public std::unary_function<TNetworkAddress*, size_t> { : size_t operator()(const TNetworkAddress* const& p) const { return hash_value(*p); } : }; : : struct TNetworkAddressPtrEquals : public std::unary_function<TNetworkAddress*, bool> { : bool operator()(const TNetworkAddress* const& p1, : const TNetworkAddress* const& p2) const { : return p1->hostname == p2->hostname && p1->port == p2->port; : } : }; these aren't use anywhere, right? grepped the be/ code and didn't see any references to them, just wanted to confirm we don't need to create a version for NetworkAddressPB 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: NetworkAddressPB MakeNetworkAddressPB(const std::string& hostname, int port); > Well this is a protobuf-specific version of the MakeNetworkAddress(hostname ahhh should we move it so its come right after "TNetworkAddress MakeNetworkAddress(const std::string& hostname, int port)" - would be clearer since it doesn't contain any docs http://gerrit.cloudera.org:8080/#/c/15822/1/common/protobuf/statestore_service.proto File common/protobuf/statestore_service.proto: http://gerrit.cloudera.org:8080/#/c/15822/1/common/protobuf/statestore_service.proto@28 PS1, Line 28: optional > Yeah, we (almost) never used 'required', see: https://stackoverflow.com/que interesting, thanks for the link -- 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: 1 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 04 May 2020 22:24:59 +0000 Gerrit-HasComments: Yes
