Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/23411 )
Change subject: KUDU-1457 [4/n] enable webserver for IPv6 ...................................................................... Patch Set 2: (1 comment) Looks good from a functionality point of view. I just favour clean enums if it is possible. Let me know what you think about my comment, whether you think that is feasible. Thanks for working on this Ashwani! http://gerrit.cloudera.org:8080/#/c/23411/2/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/23411/2/src/kudu/server/server_base.cc@620 PS2, Line 620: static bool ValidateIPConfigMode() { : return FLAGS_ip_config_mode == "ipv4" || : FLAGS_ip_config_mode == "ipv6" || : FLAGS_ip_config_mode == "dual"; : } : GROUP_FLAG_VALIDATOR(ip_config_mode, ValidateIPConfigMode); I’ve looked at server_base, and there seems to be a pattern for such validators: first parse (e.g., ParseTriStateFlag), then write a getter/validator that parses and returns the value. This way, you could also log any errors. Creating an enum here would also make things more robust. I know that in the previous revision you had enums (maybe I didn’t phrase my point clearly). My concern was that throughout this patch, once you parse the flag and create an enum, we should consistently use that enum everywhere, instead of defining other, conceptually similar enums. Sorry if I caused confusion. I’m also not entirely sure how things are included. For example, if you have a parser and validator in server_base, could that be wired up for all other places? (Maybe via a standalone header?) Let me know what you think. -- 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: 2 Gerrit-Owner: Ashwani Raina <[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: Mon, 22 Sep 2025 10:18:48 +0000 Gerrit-HasComments: Yes
