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 <din...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
