You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/12/06 09:44:06 UTC

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" binding to wildcard address 

Hello Todd Lipcon,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" binding to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" binding to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
6 files changed, 28 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
4 files changed, 90 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
4 files changed, 34 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5378/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 232:   ExternalMiniClusterOptions opts_;
nit: maybe name this cluster_opts_


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Fix: Tool remote_replica calls GetStatus RPC on a remote server which is
expected to return host/port information belonging to remote RPC server.
However, if the remote RPC was bound to wildcard ip address, the returned
host ip contains 0.0.0.0. This patch adds HostPortFromSockaddrReplaceWildcard
which helps to replace the wildcard with hostname of the remote server.

Testing: Patch makes few changes to external_mini_cluster such that it
supports binding to wildcard ip now. This is coupled with few consolidations
around bind_rpc_address_ fields which are shared between masters and
tablet servers, hence moved that up in the class hierarchy.

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
5 files changed, 97 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5378/5//COMMIT_MSG
Commit Message:

Line 20: UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)
Just passing by. The commit message doesn't explain how you fixed the bug. Could you please add that?


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
5 files changed, 97 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5378/5//COMMIT_MSG
Commit Message:

Line 20: UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)
> Just passing by. The commit message doesn't explain how you fixed the bug. 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 6:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5378/6/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

PS6, Line 82: as binding ip 
nit: "as the IP to bind to"


PS6, Line 109: UNIQUE_LOOPBACK
UNIQUE_LOOPBACK on Linux, LOOPBACK on macOS.


Line 115:   BindMode bind_mode_;
This is a struct, so per the style guide this member shouldn't have a tailing underscore: https://google.github.io/styleguide/cppguide.html#Variable_Names


Line 419:   virtual void set_rpc_bind_address(std::string address) = 0;
Why make this a virtual public method when it is only ever called by constructors?


Line 420:   virtual const std::string& get_rpc_bind_address() const = 0;
Why make this a virtual public method? Why not just a protected variable?


Line 503:   virtual void set_rpc_bind_address(std::string host) override {
Why override these methods when they are verbatim the same for both implementations?


Line 536:   virtual void set_rpc_bind_address(std::string host) override {
And these...


http://gerrit.cloudera.org:8080/#/c/5378/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 964:   opts_.bind_mode_ = ExternalMiniClusterOptions::WILDCARD;
Please add a comment noting that this line is important in order to provide test coverage for the wildcard address case.


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Fix: Tool remote_replica calls GetStatus RPC on a remote server which is
expected to return host/port information belonging to remote RPC server.
However, if the remote RPC was bound to wildcard ip address, the returned
host ip contains 0.0.0.0. This patch adds HostPortFromSockaddrReplaceWildcard
which helps to replace the wildcard with hostname of the remote server.

Testing: Patch makes few changes to external_mini_cluster such that it
supports binding to wildcard ip now. This is coupled with few consolidations
around bind_rpc_address_ fields which are shared between masters and
tablet servers, hence moved that up in the class hierarchy.

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
5 files changed, 89 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Fix: Tool remote_replica calls GetStatus RPC on a remote server which is
expected to return host/port information belonging to remote RPC server.
However, if the remote RPC was bound to wildcard ip address, the returned
host ip contains 0.0.0.0. This patch adds HostPortFromSockaddrReplaceWildcard
which helps to replace the wildcard with hostname of the remote server.

Testing: Patch makes few changes to external_mini_cluster such that it
supports binding to wildcard ip now. This is coupled with few consolidations
around bind_rpc_address_ fields which are shared between masters and
tablet servers, hence moved that up in the class hierarchy.

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
5 files changed, 89 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5378/2/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

PS2, Line 773:   // Retain the wildcard addr of "0.0.0.0" if rpc host matches
             :   // that of current host. This is needed to restart the daemons
             :   // using wildcard addresses again.
I don't follow this logic. Why are you checking the FQDN instead of just checking whether the original specified bind addr was 0.0.0.0?

eg:

if (bind_host_ != "0.0.0.0") {
  bound_rpc_ = bound_rpc_hostport();
} else {
  bound_rpc_.set_port(bound_rpc_hostport().port());
}

(not sure those are exactly the right methods but you get the point)


PS2, Line 1055: ("--rpc_bind_addresses=$0:$1",
              :                              bound_rpc_.host(), bound_rpc_.port())
what effect does this have? isn't it equivalent?


Line 1061:                              bound_http_.host()));
was this change necessary?


http://gerrit.cloudera.org:8080/#/c/5378/2/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

PS2, Line 126: bind to hostname
not sure what 'bind to hostname' means. This should also specify whether it affects RPC or webserver or both. Also please specify which takes precedence between this and 'bind_to_unique_loopback_addresses' above. Perhaps it would be better to change the latter boolean to be an enum like BindMode { LOOPBACK, UNIQUE_LOOPBACK, WILDCARD }?


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 8:

(1 comment)

a

http://gerrit.cloudera.org:8080/#/c/5378/8/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

Line 440:   virtual const std::string& get_rpc_bind_address() const {
nit: no need for virtual here and below


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 8: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Fix: Tool remote_replica calls GetStatus RPC on a remote server which is
expected to return host/port information belonging to remote RPC server.
However, if the remote RPC was bound to wildcard ip address, the returned
host ip contains 0.0.0.0. This patch adds HostPortFromSockaddrReplaceWildcard
which helps to replace the wildcard with hostname of the remote server.

Testing: Patch makes few changes to external_mini_cluster such that it
supports binding to wildcard ip now. This is coupled with few consolidations
around bind_rpc_address_ fields which are shared between masters and
tablet servers, hence moved that up in the class hierarchy.

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Reviewed-on: http://gerrit.cloudera.org:8080/5378
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/log-rolling-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
5 files changed, 89 insertions(+), 41 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5378/2/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

PS2, Line 773:   // start again we'll reuse these. Store only the port if the
             :   // daemons were using wildcard address for binding.
             :   const string& wildcard_ip = Substi
> I don't follow this logic. Why are you checking the FQDN instead of just ch
yeah, that could have been a simpler thing to do. bind_host_ belongs to a derived class, so while doing that, I ended up consolidating bind_host_ member between base class and derived classes. Diffs became more invasive than I originally intended, pls let me know if those cleanups weren't necessary.


PS2, Line 1055: 
              :   flags.push_back("--fs_wal_dir=" + data_dir_);
> what effect does this have? isn't it equivalent?
It is equivalent, and I think I was trying to use bound_rpc_ in a different way to restart tserver with wildcard but forgot to revert this line later. Updated with new code here.


Line 1061:   flags.push_back(Substitute("--webserver_port=$0", bound_http_.port()));
> was this change necessary?
Not strictly necessary, it should ideally be using the one it had stored under bound_http_ in the shutdown path to start the webserver on the same ip address when server is restarted. Earlier, it just so happened that bound_http_ had same host address as bind_host_, so it has been working without any issues I think.


http://gerrit.cloudera.org:8080/#/c/5378/2/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

PS2, Line 126: 
> not sure what 'bind to hostname' means. This should also specify whether it
Yeah I messed up that comment, I was thinking of a quick doc explaining how 0.0.0.0 end up resolving to a specific interface ip via GetFQDN etc.

I took your suggestion of making this an enum and updated the patch, also update the comments around enum now.


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/5378/6/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:

PS6, Line 82: as binding ip 
> nit: "as the IP to bind to"
Done


PS6, Line 109: UNIQUE_LOOPBACK
> UNIQUE_LOOPBACK on Linux, LOOPBACK on macOS.
Done


Line 115:   BindMode bind_mode_;
> This is a struct, so per the style guide this member shouldn't have a taili
good catch, done.


Line 419:   virtual void set_rpc_bind_address(std::string address) = 0;
> Why make this a virtual public method when it is only ever called by constr
Good point,done.


Line 420:   virtual const std::string& get_rpc_bind_address() const = 0;
> Why make this a virtual public method? Why not just a protected variable?
Done


Line 503:   virtual void set_rpc_bind_address(std::string host) override {
> Why override these methods when they are verbatim the same for both impleme
yeah, defeats the the purpose altogether, i moved them to base class.


Line 536:   virtual void set_rpc_bind_address(std::string host) override {
> And these...
Done


http://gerrit.cloudera.org:8080/#/c/5378/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

Line 232:   ExternalMiniClusterOptions opts_;
> nit: maybe name this cluster_opts_
Done


Line 964:   opts_.bind_mode_ = ExternalMiniClusterOptions::WILDCARD;
> Please add a comment noting that this line is important in order to provide
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................

KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address

"remote_replica copy" asks the source server for its bound address,
but that address could be wildcard (0.0.0.0) in a real cluster,
and if same address is stuffed in RPC, it fails the validation
at the source tablet server:

tablet_id: "834e7673f32e4802bdf26f9fff9162ef"
copy_peer_uuid: "893679e1b1dd4342bf1fb8f058ec2789"
copy_peer_addr {
    host: "0.0.0.0"
    port: 7050
}
UNKNOWN_ERROR: Invalid wildcard address to tablet copy from: 0.0.0.0 (resolved to 0.0.0.0)

Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
---
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
4 files changed, 98 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5378/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5378/1//COMMIT_MSG
Commit Message:

PS1, Line 7: binding to wildcard address
> the tool isn't binding to a wildcard, but rather trying to connect to a wil
Done


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 68: DEFINE_bool(ts_use_wildcard_addr, false,
> this should be one of the options set via the minicluster API before starti
yeah, good idea, done. also keeping it only for tablet server for now.


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:

Line 269: Status HostPortPBFromSockaddrReplaceWildcard(const Sockaddr& addr, HostPortPB* hp) {
> despite this being convenient, it's a "no-no" to have util/ depend on commo
that's true, I kinda wanted to poke you about this change in the slack, but forgot about that. thanks for steering me towards a sane change.


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:

PS1, Line 118: HostPortHostPortPB
> typo
all diffs from kudu/util/net got removed now following your suggestion below.


Line 121: Status HostPortPBFromSockaddrReplaceWildcard(const Sockaddr& addr, HostPortPB* hp);
> why not just use HostPortFromSockaddr as above, and then convert it to a PB
Done.


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" connecting to wildcard address

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" connecting to wildcard address
......................................................................


Patch Set 8:

Let's pull this in for 1.2.0

-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1776: Fix "kudu remote replica copy" binding to wildcard address

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1776: Fix "kudu remote_replica copy" binding to wildcard address
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5378/1//COMMIT_MSG
Commit Message:

PS1, Line 7: binding to wildcard address
the tool isn't binding to a wildcard, but rather trying to connect to a wildcard


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/integration-tests/external_mini_cluster.cc
File src/kudu/integration-tests/external_mini_cluster.cc:

Line 68: DEFINE_bool(ts_use_wildcard_addr, false,
this should be one of the options set via the minicluster API before starting it, rather than a gflag


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:

Line 269: Status HostPortPBFromSockaddrReplaceWildcard(const Sockaddr& addr, HostPortPB* hp) {
despite this being convenient, it's a "no-no" to have util/ depend on common/ since it's a cyclic dependency. Also util/ should be general utility code whereas common/ is full of very Kudu-specific stuff.


http://gerrit.cloudera.org:8080/#/c/5378/1/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:

PS1, Line 118: HostPortHostPortPB
typo


Line 121: Status HostPortPBFromSockaddrReplaceWildcard(const Sockaddr& addr, HostPortPB* hp);
why not just use HostPortFromSockaddr as above, and then convert it to a PB?


-- 
To view, visit http://gerrit.cloudera.org:8080/5378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5d0a37b39a3774caab5b5d8dba3d9750bf5f21f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes