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 8: Code-Review+1 (6 comments) PS11 looks better. Just a few nits and questions. Thanks a lot for revving this! 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: > This is just one time usage here. Even though similar sort manipulation is Thanks for checking this out. 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: verTest { Thanks a lot for the explanation. > Shouldn't there be coverage for the 'dual' mode well? Whoops, for some reason, I wrote 'dual' there instead of 'ipv6' -- not sure why. > In theory, we could have all three modes for all the tests, but it would not > serve much purpose. I guess there might be a need to have strict 'ipv6' mode tests if we want to make sure that nothing but 'ipv6' addresses are used if both 'ipv4' and 'ipv6' stacks are available. If you know this isn't something that we need to test for as of now, having just 'ipv4' and 'dual' should be enough, I agree. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@1017 PS5, Line 1017: > I am not really keen on crashing with SIGABRT. I guess it would require changing the signature of this method to start using FAIL() here instead. If that's too much, and given that in PS8 there is also CHECK_OK() for ParseIPModeFlag(), having CHECK() might be the most straightforward way of updating this existing code. It's up to you -- this is just a nit after all, and I'm fine if keeping CHECK() here as-is. 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 > This aligns with --rpc_bind_addresses that is also set to default > 'unspecified' address. Right, but that was for IPv4 only. I'm just pointing to the fact that with introduction of the IP config mode concept it would make sense to keep this flag's default value empty and change its semantics correspondingly, and maybe change the rpc_bind_addresses's default value to an empty string as well, with semantics of binding to 'wildcard' address (whatever it means for an IP config mode) if a flag is set to an empty string. With that, it would be more consistent with any setting of the --ip_config_mode flag. What do you think? http://gerrit.cloudera.org:8080/#/c/23411/8/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/23411/8/src/kudu/util/net/net_util.cc@170 PS8, Line 170: Status ParseIPModeFlag(const char* flag_name, : const string& flag_value, : IPMode* mode) { : if (iequals(flag_value, "ipv4")) { : *mode = IPMode::IPV4; : } else if (iequals(flag_value, "ipv6")) { : *mode = IPMode::IPV6; : } else if (iequals(flag_value, "dual")) { : *mode = IPMode::DUAL; : } else { : return Status::InvalidArgument( : Substitute("$0: invalid value for flag --$1, can be one of " : "'ipv4', 'ipv6', 'dual'; case insensitive", : flag_value, flag_name)); : } : return Status::OK(); : } Do we expect to the ip_config_mode flag change in runtime? If not, consider having a function that parses the flag just once when the function is called first time (e.g., it's possible to use std::once for that) and then reuse the result on follow-up calls. Also, do we need to have this parameterized by flag name at all? If this isn't used elsewhere for a different flag, why to have these two first parameters? http://gerrit.cloudera.org:8080/#/c/23411/8/src/kudu/util/net/net_util.cc@515 PS8, Line 515: if (PREDICT_FALSE(out == nullptr)) { : return Status::InvalidArgument("null output string"); : } nit: usually, this is done using DCHECK() -- since the variability of the nullness for the 'out' parameter comes from the code (not from the data), it's enough to have DCHECK() to catch programming errors -- 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: 8 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: Fri, 03 Oct 2025 08:38:21 +0000 Gerrit-HasComments: Yes
