Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23411 )
Change subject: KUDU-1457 [4/n] enable webserver for IPv6 ...................................................................... Patch Set 5: (20 comments) First quick review round. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master-test.cc File src/kudu/master/mini_master-test.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master-test.cc@40 PS5, Line 40: class MiniMasterTest : public KuduTest, : public ::testing::WithParamInterface<std::string> { : protected: : static std::string get_host() { : FLAGS_ip_config_mode = GetParam(); : if (FLAGS_ip_config_mode == "ipv6") { : return "::1"; : } : return "127.0.0.1"; : } : }; : : // This is used to run all parameterized tests with different IP modes. : INSTANTIATE_TEST_SUITE_P(Parameters, MiniMasterTest, : testing::Values("ipv4", "ipv6")); Shouldn't there be coverage for the 'dual' mode here as well? Most likely I'm missing something here, but if not adding such coverage in this test, where are you going to have the 'dual' mode covered then? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/master/mini_master.cc@70 PS5, Line 70: "[" + web_bind_addr.host() + "]" Shouldn't this be somehow encapsulated into HostPort or just a stand-alone method in network utilitues instead of doing this manually with all the boiler plate in all the places? Maybe, add a new 'ToIpInterface()' method into HostPort for this? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@188 PS5, Line 188: "ipv4", "dual" Why 'ipv6' is omitted? Could you at least add a comment about the reason? Shouldn't there be coverage for the 'dual' mode well? Most likely I'm missing something, but I'm not sure I saw a single test scenario where the 'dual' mode would be covered. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@288 PS5, Line 288: "ipv4", "dual" ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@299 PS5, Line 299: "ipv4", "dual" ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@425 PS5, Line 425: ipv4", "dual ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@526 PS5, Line 526: pv4", "dual ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@539 PS5, Line 539: ipv4", "dual ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@744 PS5, Line 744: ipv4", "dual ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@753 PS5, Line 753: ipv4", "dual ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@830 PS5, Line 830: "ipv4", "dual") ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@893 PS5, Line 893: "ipv4", "dual" ditto regarding the omitted 'ipv6' http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@994 PS5, Line 994: "127.0.0.1" nit: why not to use expected_advertised_host_ here as well? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@1017 PS5, Line 1017: CHECK(false) nit: would calling FAIL() and returning an empty string fit here, or you explicitly want it to crash with SIGABRT in such cases? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver_options.cc@47 PS5, Line 47: 0.0.0.0 Does this implicitly assume IPv4 then? Why not to keep the original semantics of an empty default string here, and bind to all available IPv4 and IPv6 interfaces if the IP config mode is 'dual'? Correspondingly, for 'ipv4' config mode this would mean '0.0.0.0', but for 'ipv6' config mode this would mean '::/0'? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@88 PS5, Line 88: Value is case-sensitive. Why? Being case-sensitive it's not user-friendly, and many of our string flags are case-insensitive, so it's a bit off this way. Or that's a typo? I see that ParseIPModeFlag() in PS5 uses iequals(). http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@115 PS5, Line 115: Status ParseIPModeFlag(const string& name, Why to have this as a standalone function if it's only used in ValidateIPConfigMode()? Just inline it instead? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@124 PS5, Line 124: "IP config mode cannot be any value other than 'ipv4' " : "or 'ipv6' or 'dual'. Options are case in-sensitive." nit: replace with can be one of 'ipv4', 'ipv6', 'dual'; case insensitive" http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@365 PS5, Line 365: FLAGS_ip_config_mode == "ipv4" Here and elsewhere where this flag is involved: since the value for the flag is case-insensitive, this way of comparison is brittle and isn't going to work if they specify it as 'IPv4' or similar. Instead, consider using the more robust approach similar to ParseTriState() and storing the result as an enum field or something like that. Also, there is also 'sa_family_t' family parameter, why it's not used here instead? http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/util/net/net_util.cc@372 PS5, Line 372: hints.ai_family = family Is this consistent if FLAGS_ip_config_mode == 'dual'? -- To view, visit http://gerrit.cloudera.org:8080/23411 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7095561539f427b5c58ada7b480ca549b0ab795e Gerrit-Change-Number: 23411 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 30 Sep 2025 03:29:15 +0000 Gerrit-HasComments: Yes
