Lars Volker has posted comments on this change. Change subject: IMPALA-3394: Add tests, make BackendConfig own class, refactor ......................................................................
Patch Set 1: (8 comments) Thanks for the review, please see PS2 and my comments. http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/scheduling/backend-config-test.cc File be/src/scheduling/backend-config-test.cc: Line 31: vector<TNetworkAddress> backends; > you can use a {} c'tor Done Line 62: const auto& backend_list = backend_config.GetBackendListForHost("10.0.0.1"); > auto only for iterators Done http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/scheduling/backend-config.cc File be/src/scheduling/backend-config.cc: Line 56: BackendList* be_descs = &backend_map_[be_desc.ip_address]; > you can use a ref here Done http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/scheduling/backend-config.h File be/src/scheduling/backend-config.h: Line 74: typedef std::shared_ptr<const BackendConfig> BackendConfigPtr; > best left to the user of this class Done http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 217: for (const auto& backend: backends) { > auto only for iterators Done http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/util/network-util.cc File be/src/util/network-util.cc: Line 159: > remove one blank line Done http://gerrit.cloudera.org:8080/#/c/4116/1/be/src/util/network-util.h File be/src/util/network-util.h: Line 27 > this only ever returned a single address? No, but it was only used in the scheduler, and at the only place where it was used only one address was extracted from the result. I moved that logic here now, too. Line 55: TBackendDescriptor MakeBackendDescriptor(const Hostname& hostname, const IpAddr& ip, > output param I tried to follow the surrounding code here. IMO changing this to an output parameter will make the calling code less readable. The copy of the return value can be elided by the compiler. Should I change it nonetheless? -- To view, visit http://gerrit.cloudera.org:8080/4116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2d3acb6f68b16ca0af06dad0098d7ec1eff41202 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
