Ashwani Raina 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 The idea is to simply rely on the flag instead of hard-coded value. Even if this is only for INET addresses, we can rely on flag instead of blindly setting the option to true. 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 call I left out the check in first PS with same understanding. But I was not sure whether ReserveDaemonSocket can ever be used for Unix domain sockets (GetBindIpForDaemon like method for Unix domain address and corresponding parsing method). I guess we can skip this check for now and rely on inet_pton() inside ParseString() to do the check for us. 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 Done 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 namesp Done 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' Done -- 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: Wed, 28 May 2025 16:27:11 +0000 Gerrit-HasComments: Yes
