Tim Armstrong 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: (8 comments) 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 TServerSocket is a subclass of TServerTransport - I needed to use the subclass here for getPort() - https://github.com/apache/thrift/blob/0.9.3/lib/cpp/src/thrift/transport/TServerSocket.h#L116 So yeah, shouldn't limit us at all. 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..." I think I meant the word port, but referring to the variable is probably clearer. 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? I'm not sure why I did this... reverted. 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 Done 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. Done 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 Thanks for catching this. 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 surroundi It's protected by the lock now, since it's modified in start rather than constant for the lifetime of the object. It was only ever referenced under the lock before anyway. http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/10740/2/be/src/util/network-util.cc@196 PS2, Line 196: server_address.sin_port = htons(port); > I think we can set this to 0 and bind to a random port instead of looping t Nice catch! I hadn't really thought about it. We talked out of band and my concern was that doing this might increase the risk of collisions. But it sounds like it probably makes things better because linux apparently allocates them pseudo-sequentially: https://eklitzke.org/binding-on-port-zero . That whole blog post is really relevant. I think you said you were going to look at doing it, but I can also do as a follow-on if needed. -- 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-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Wed, 20 Jun 2018 16:10:35 +0000 Gerrit-HasComments: Yes
