Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19938 )
Change subject: KUDU-3477 Function to get flags of minicluster daemons. ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/kudu-tool-test.cc@7794 PS3, Line 7794: //Check nit: add a space. http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/kudu-tool-test.cc@7803 PS3, Line 7803: ASSERT_EQ(resp.get_daemon_flags().flags().size(), 1); : ASSERT_EQ(resp.get_daemon_flags().flags()[0].name(), "rpc_negotiation_timeout_ms"); : ASSERT_EQ(resp.get_daemon_flags().flags()[0].value(), "5000"); If check failed, gtest will output a message like: Expected: resp.get_daemon_flags().flags().size() Which is: 2 // for example To be equal to: 1 Which is: 1 So how about place the parameters order as ASSERT_EQ(expected, actual)? http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/tool.proto@213 PS3, Line 213: optional bool all_flags = 2; : // A list of flag tags. Flags that match at least one tag are returned. If : // no tags are specified, all flags match. : repeated string tags = 3; : // A list of flags. Flags that in this list are returned. If flags are specified, : // will ignore 'all_flags'. If not, all flags match. : repeated string flags = 4; It's duplicate to server_base.GetFlagsResponsePB, is it possible to reuse GetFlagsResponsePB? For example, use it as a member variable of GetDaemonFlagsRequestPB. http://gerrit.cloudera.org:8080/#/c/19938/3/src/kudu/tools/tool.proto@223 PS3, Line 223: message Flag { : optional string name = 1; : optional string value = 2; : repeated string tags = 3; : // true if the flag has its default value. : optional bool is_default_value = 4; : } : : repeated Flag flags = 1; How about reusing GetFlagsResponsePB.Flag ? -- To view, visit http://gerrit.cloudera.org:8080/19938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a Gerrit-Change-Number: 19938 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 12 Jun 2023 08:06:19 +0000 Gerrit-HasComments: Yes
