Alexey Serbin 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 below and this IPV6_V6ONLY option. Is there some rationale to have such a difference? Why not to return back Status::ConfigurationError() as well if IPV6_V6ONLY is attempted to set on a Unix socket? 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); > It is important to have a control over what type of sockets we create in or Right, it's important to have unit-style tests like this. However, do you think we will need to eventually add coverage for the the whole stack end-to-end when setting particular IP mode for the server side? I also don't expect too many surprises, but I agree with Marton that it should be great to have it covered at least to catch regressions, if any. 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? 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 method that would be called without this overriding TearDown() method? 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 it improves readability and brings less surprises 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? 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 avoid shadowing the variable in the second sub-scope? 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 ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); 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: ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); -- 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: Tue, 16 Dec 2025 08:01:15 +0000 Gerrit-HasComments: Yes
