Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/23651 )
Change subject: KUDU-1457 [5/n] Enable IPv6-only mode for rpc ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/messenger.cc@267 PS2, Line 267: // IPV6_V6ONLY socket option is not applicable to Unix domain sockets. > It seems there is a difference in behavior between setting SO_REUSEPORT bel My intention was to allow further socket operations on a Unix domain socket even if the ip_config_mode is set to "ipv6". ip_config_mode is a server level setting and not a per socket configuration. In other words, it should be ok to have ip_config_mode set to "ipv6" and be able to create Unix domain sockets. For example, using --rpc_listen_on_unix_domain_socket flag, if a RPC server is initialized to listen on a Unix domain socket, and if ip_config_mode happens to be set to "ipv6", the RPC server init will fail if we return with a config error. reuseport is set on a messenger object that can potentially have multiple acceptor pools with dedicated socket and each socket can have distinct options set. Whereas ip_config_mode is set at server level and with this set to "ipv6", co-existence of Unix domain sockets and TCP sockets is possible (if such a requirement arises). Maybe such a use case is not valid even for tests. Since FLAGS_rpc_reuseport is also set at server level i.e. it will be applicable to all sockets under a messenger object. I guess there is not much different between the two scenarios. It makes sense to change this to reuseport like case and simply return Status::ConfigurationError() as you pointed out. I have added a similar check in ValidateSocketOptionCompatibility() flag validator to report incompatibility between FLAGS_rpc_listen_on_unix_domain_socket and ip_config_mode set to 'ipv6' http://gerrit.cloudera.org:8080/#/c/23651/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/23651/1/src/kudu/rpc/rpc-test.cc@2326 PS1, Line 2326: s = s6.Connect(target_addr); > Right, it's important to have unit-style tests like this. I can imagine some regression scenarios might arise in future where some RPC code modifications can lead to this socket option not getting set for some reason, especially from calling layers sitting above Socket class. In such cases, we would definitely need some end-to-end tests to catch such scenarios. That additional set of tests would require additional customization code that is out of scope of this patch. I am happy to do it in a separate patch or upstream jira. http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2247 PS2, Line 2247: CHECK_OK > Why not to use ASSERT_OK here? ASSERT_OK returns status from ctor. I have removed the ctor and replaced with SetUp() along with ASSERT_OK. http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2258 PS2, Line 2258: void TearDown() override { : RpcTestBase::TearDown(); : } > What are you trying to achieve with this override if it simply calls the me No idea why it was there. Removed. http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2271 PS2, Line 2271: Status s; > nit: when possible, having variables with smaller scope is preferred since Done http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2277 PS2, Line 2277: TestRpc.TestIpConfigModes > This doesn't seem to match the name of the test? Done http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2286 PS2, Line 2286: // Target address is required mainly for dual mode. This is required to rule out : // any possibility of 'connect' call failing due to invalid target address. : Sockaddr target_addr = server_addr; > Move this under the first sub-scope with IPv4-related client scenarios to a Done http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2306 PS2, Line 2306: ASSERT_FALSE(s.ok()); > Consider be more explicit on the type of Status error, e.g. something like Done http://gerrit.cloudera.org:8080/#/c/23651/2/src/kudu/rpc/rpc-test.cc@2329 PS2, Line 2329: ASSERT_FALSE(s.ok()); : ASSERT_STR_CONTAINS(s.ToString(), "Invalid argument"); > Replace these two lines with one: The error string : Network error: connect(2) error: Invalid argument (error 22) "Invalid argument" is the internal error (errno) and the one that is returned back to callers is kNetworkError. How about this, check the error code as well as errno from status string. It aligns with above check at lines 2306-2307: ASSERT_TRUE(s.IsNetworkError()) << s.ToString(); ASSERT_STR_CONTAINS(s.ToString(), "Invalid argument"); -- To view, visit http://gerrit.cloudera.org:8080/23651 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic84ba301d0faa36e928eec54872d02f28a4f793f Gerrit-Change-Number: 23651 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 29 Dec 2025 09:34:56 +0000 Gerrit-HasComments: Yes
