Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10740 )
Change subject: IMPALA-7151: Rework ephemeral port assignment for be tests ...................................................................... Patch Set 2: (7 comments) Thanks for doing this. This also sets up some of the code nicely for IMPALA-6013 which I'll be getting to soon. http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/rpc/thrift-server.cc File be/src/rpc/thrift-server.cc: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/rpc/thrift-server.cc@366 PS2, Line 366: TServerSocket Are we sure that this change from TServerTransport to TServerSocket doesn't handicap us from using TServerTransport APIs that we may find useful in the future? Or were we doing the wrong thing by using TServerTransport all along? http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/rpc/thrift-server.cc@452 PS2, Line 452: If port was 0 nit: "If port_ was initially configured as 0..." http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/runtime/exec-env.h@222 PS2, Line 222: TNetworkAddress configured_backend_address_; Why make this public? http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/scheduling/scheduler.h@96 PS2, Line 96: UpdateLocalBackendForBeTest UpdateLocalBackendAddrForBeTest http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/service/impala-server.h@142 PS2, Line 142: ///respective service will not be started. Update comment with new behavior about BE tests passing in port=0. http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/service/impala-server.cc@2070 PS2, Line 2070: >= > 0 http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/statestore/statestore-subscriber.h File be/src/statestore/statestore-subscriber.h: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/statestore/statestore-subscriber.h@190 PS2, Line 190: /// Address that the heartbeat service should be started on. Initialised in constructor, : /// updated in Start() with the actual port if the wildcard port 0 was specified. : TNetworkAddress heartbeat_address_; Any reason this was moved down? Prefer leaving it where it was as surrounding members are similar there. -- To view, visit http://gerrit.cloudera.org:8080/10740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2fcc9243099f8249380ac5f01ac5eb67cba24cf5 Gerrit-Change-Number: 10740 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 19 Jun 2018 22:57:32 +0000 Gerrit-HasComments: Yes
