You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2023/05/25 13:10:20 UTC

[kudu-CR] [WIP] KUDU-3477 Function to get flags of minicluster daemons.

zmartonka@cloudera.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19938


Change subject: [WIP] KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

[WIP] KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
5 files changed, 101 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/1
-- 
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: newchange
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>

[kudu-CR] [WIP] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19938

to look at the new patch set (#2).

Change subject: [WIP] KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

[WIP] KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
5 files changed, 103 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/2
-- 
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: newpatchset
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Jun 2023 08:06:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
zmartonka@cloudera.com 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 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/19938/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/19938/1/src/kudu/mini-cluster/external_mini_cluster.cc@103
PS1, Line 103: using kudu::server::GetFlagsRequestPB;
Is it ok to expect proto messages as the function parameters, or should this structures be duplicated with a plain c++ object?


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@7803
PS3, Line 7803: auto* r = req.mutable_get_daemon_flags();
              :     *r->mutable_id() = tservers[0].id();
              :     r->mutable_request()->add_flags("rpc_negotiation_timeout_ms");
> If check failed, gtest will output a message like:
Thank you.


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: // The actual request
             :   optional kudu.server.GetFlagsRequestPB request = 2;
             : }
             : 
             : // Call SetFlag() on the specific daemon.
             : message SetDaemonFlagRequestPB {
             :   // The identifier of the d
> It's duplicate to server_base.GetFlagsResponsePB, is it possible to reuse G
It is possible, but it will include a dependency of tools_proto -> server_base_proto. That's why I wanted to duplicate it.
If this new dependency is acceptable, then it indeed makes the changes smaller.
I don't know if introducing this dependency has any negative consequences, probably not.
I updated the code review. I can revert the proto back if the dependency is bad.



-- 
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: 6
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Jun 2023 10:55:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19938

to look at the new patch set (#4).

Change subject: KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 72 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/4
-- 
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: newpatchset
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19938

to look at the new patch set (#5).

Change subject: KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 73 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/5
-- 
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: newpatchset
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Yingchun Lai, Attila Bukor, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19938

to look at the new patch set (#6).

Change subject: KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 73 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/6
-- 
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: newpatchset
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Marton Greber (Code Review)" <ge...@cloudera.org>.
Marton Greber 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 6:

> Patch Set 6:
> 
> (3 comments)

Regarding the functionality it looks good to me!

Small addition:
- could you please add the corresponding Java side implementation for 'getMasterFlags' and 'setMasterFlags' in MiniKuduCluster.java?
- Add some tests in TestMiniKuduCluster.java (both for getting master and tserver flags):
   * check that we get non-zero number of flags,
   * check that a given default flag is present in the flag list,
   * set a test specific flag, and check that we get it back.
(test specific flags are set like:
  @MasterServerConfig(flags = {
      "--master_client_location_assignment_enabled=true",
  })
  public void testClientLocation() throws Exception {
)

Thanks!


-- 
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: 6
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Jul 2023 21:25:09 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19938

to look at the new patch set (#3).

Change subject: KUDU-3477 Function to get flags of minicluster daemons.
......................................................................

KUDU-3477 Function to get flags of minicluster daemons.

Having a call which returns flags set for a given daemon in a minicluster will
be useful for some additional testing when upgrading the python client. This
functionality is already present in server_base.proto (GetFlagsRequestPB),
only the layer for the minicluster is missing.

Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
5 files changed, 103 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/19938/3
-- 
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: newpatchset
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <zm...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu 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 6: Code-Review+1


-- 
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: 6
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jul 2023 10:24:07 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3477 Function to get flags of minicluster daemons.

Posted by "Zoltan Martonka (Code Review)" <ge...@cloudera.org>.
Zoltan Martonka has abandoned this change. ( http://gerrit.cloudera.org:8080/19938 )

Change subject: KUDU-3477 Function to get flags of minicluster daemons.
......................................................................


Abandoned
-- 
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: abandon
Gerrit-Change-Id: I3480bfccce18c4605ed9b4ed1dab367fa1fd525a
Gerrit-Change-Number: 19938
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Martonka <zm...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Zoltan Martonka <zm...@cloudera.com>