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 2: (5 comments) 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: > nit: doc? - I think the equivalent for TBackendId is "// Used to uniquely i Right, no typedefs in protobuf. I also moved this to a better place. http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/scheduling/cluster-membership-test-util.h File be/src/scheduling/cluster-membership-test-util.h: http://gerrit.cloudera.org:8080/#/c/15822/1/be/src/scheduling/cluster-membership-test-util.h@20 PS1, Line 20: #include <string> > still needed? Done. I also went through and cleaned up some more similar includes. 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: > nit: doc - I guess this is slightly different from MakeNetworkAddress since Well this is a protobuf-specific version of the MakeNetworkAddress(hostname, port) a little further up. 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" // for UniqueIdPB > nit: add " // for UniqueIdPB" Done 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 i Yeah, we (almost) never used 'required', see: https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3 -- 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: 2 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: Fri, 01 May 2020 17:00:03 +0000 Gerrit-HasComments: Yes
