Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19393 )
Change subject: [KUDU-3430] Implement getting flags filter logic in tool side ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/19393/4/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/19393/4/src/kudu/server/generic_service.cc@49 PS4, Line 49: filter_logic nit: What filter logic? Maybe have to mention 'gflag' and 'on server'? http://gerrit.cloudera.org:8080/#/c/19393/4/src/kudu/server/generic_service.cc@95 PS4, Line 95: auto* flag = resp->add_flags(); : flag->set_name(entry.first); : flag->set_value(CheckFlagAndRedact(entry.second, EscapeMode::NONE)); : flag->set_is_default_value(entry.second.current_value == entry.second.default_value); : for (const auto& tag : tags) { : flag->add_tags(tag); : } These code is duplicate, how about improved by: if (!FLAGS_disable_filter_logic_for_testing) { // filter logoc ... if (!matches) continue; } ... http://gerrit.cloudera.org:8080/#/c/19393/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19393/4/src/kudu/tools/kudu-tool-test.cc@7817 PS4, Line 7817: for (int i = 0; i <= 1; i++) What does 0 and 1 mean? How about use TEST_P instead of TEST_F in this case? -- To view, visit http://gerrit.cloudera.org:8080/19393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a2ddc2540d7aa70d3dd2e6c4101bb227e94c858 Gerrit-Change-Number: 19393 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Mon, 09 Jan 2023 06:49:14 +0000 Gerrit-HasComments: Yes
