Ashwani Raina 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: (5 comments) 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. For now, 'ipv4' and 'dual' should be good enough. I am planning to add more tests but that will happen later. Current test coverage is to ensure basic functionality is working fine. http://gerrit.cloudera.org:8080/#/c/23411/5/src/kudu/server/webserver-test.cc@1017 PS5, Line 1017: > I guess it would require changing the signature of this method to start usi Changing the signature would involve more cycles and I have other priority work that needs urgent attention. Would prefer to keep it 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 'unspec I am thinking from different perspective though: Default value for --ip_config_mode is ipv4, default for these two flags is set to INADDR_ANY (any address in ipv4 notation). That kind of fits the bill here. If you are not too keen on moving back the default values to empty string, I would like to keep this as is. Moving back to empty string for both flags would require more cycles at this point and I don't think I have any given the list of other pending items. Unless you see an issue here that needs to be fixed in any case, let's keep this as-is. 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, conside It is not expected to change at runtime. I had thought about using std::once or something similar but that puts a restriction on tests that extensively change the flag value to test out different modes. I guess we can make tests run with std:once usage and have some sort of reset logic that resets the value in order for tests to be able to set them again. This requires more time to get it right. For now, I would like to make a note of this and address this later. This gets called from validator function as well which does validation before setting the value. So at least input value needs to be passed here. Since this is used for just one flag i.e. ip_config_mode, I have removed the first argument i.e. flag_name 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 n Makes sense. Done. -- 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 13:51:23 +0000 Gerrit-HasComments: Yes
