Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61 PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport; Why to change this at all? Given the signature of this constructor, I'd be surprised if 'rpc_bind_addr' were something else but INET address. http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc@72 PS3, Line 72: // SO_REUSEPORT socket option is not applicable to Unix domain sockets. By definition, MiniCluster::ReserveDaemonSocket() isn't supposed to be called for non-INET sockets. That's also clear from the call to GetBindIpForDaemonWithType() and from the signature of the method (it has the 'port' parameter). If that were done, it would be a programming error, and it would error out at least in sock_addr.ParseString() anyway. With that, sock_addr.is_unix() cannot be 'true' at line 73, so there is no need to add any checks like this. http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc@263 PS3, Line 263: if (reuseport_ && accept_addr.is_unix()) { : return Status::ConfigurationError("Port reuse is not applicable to Unix domain sockets."); : } else if (reuseport_) { : RETURN_NOT_OK(sock.SetReusePort(true)); : } nit: from the readability perspective, please consider re-structuring this piece similar to the code below if (reuseport_) { if (PREDICT_FALSE(accept_addr.is_unix())) { return Status::ConfigurationError(...); } RETURN_NOT_OK(sock.SetReusePort(true)); } http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@516 PS3, Line 516: static nit: no need to add 'static' since this function is in the anonymous namespace already http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@520 PS3, Line 520: Unix domain socket nit: 'a Unix domain socket' or 'Unix domain sockets' -- To view, visit http://gerrit.cloudera.org:8080/22882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21 Gerrit-Change-Number: 22882 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 27 May 2025 17:29:30 +0000 Gerrit-HasComments: Yes
