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: (4 comments) went through part of it, few initial comments. haven't gone through all of it yet. http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/runtime/exec-env.h@74 PS1, Line 74: typedef UniqueIdPB BackendIdPB; nit: doc? - I think the equivalent for TBackendId is "// Used to uniquely identify individual impalads." I guess protobuf does not support typedefs in the .proto file? 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); nit: doc - I guess this is slightly different from MakeNetworkAddress since it takes in a port as well? http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/unique-id-hash.h File be/src/util/unique-id-hash.h: http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/util/unique-id-hash.h@25 PS1, Line 25: #include "gen-cpp/common.pb.h" nit: add " // for UniqueIdPB" 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 most of the "required" fields from the thrift definition are now optional in the protobuf defintion, was that intentional? -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Tue, 28 Apr 2020 21:38:40 +0000 Gerrit-HasComments: Yes
