Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15893 )
Change subject: KUDU-3112. Fix WaitForBind method for checking service status ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/15893/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15893/3//COMMIT_MSG@13 PS3, Line 13: but when we pass a specific address to the method, : e.g. 127.0.0.1, and the 'lsof' command show 'n*:<port-num>' Right, and there was an idea to find the appropriate socket. I.e., if a process binds to many addresses and ports, specifying particular address means we know that the process will open a socket on that address and want to wait until it happens. It might be the case that a process opens two sockets: one at *:1000 and another 127.0.0.1:2000, and we are interested to wait for the latter one. I'm not sure it's safe to change the behavior of this method as implemented in this patch. Maybe, we should clarify why Ranger starts listening on all interfaces when we expect it to listen only on 127.0.0.1? Attila, Hao, what do you guys think? http://gerrit.cloudera.org:8080/#/c/15893/3/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/15893/3/src/kudu/util/test_util.cc@445 PS3, Line 445: string all_pattern = "n*:"; nit: add 'static const' http://gerrit.cloudera.org:8080/#/c/15893/3/src/kudu/util/test_util.cc@446 PS3, Line 446: string addr_pattern = (!addr || *addr == "0.0.0.0") ? "" : Substitute("n$0:", *addr); nit: add 'const' http://gerrit.cloudera.org:8080/#/c/15893/3/src/kudu/util/test_util.cc@446 PS3, Line 446: string addr_pattern = (!addr || *addr == "0.0.0.0") ? "" : Substitute("n$0:", *addr); I think it would be simpler if we had: const string addr_pattern = (!addr || *addr == "0.0.0.0") ? all_pattern : Substitute("n$0:", *addr); http://gerrit.cloudera.org:8080/#/c/15893/3/src/kudu/util/test_util.cc@458 PS3, Line 458: if (!cur_line.contains("->")) { : if (HasPrefixString(cur_line.ToString(), all_pattern)) { : cur_line.remove_prefix(all_pattern.size()); : } else if ((!addr_pattern.empty()) && : HasPrefixString(cur_line.ToString(), addr_pattern)) { : cur_line.remove_prefix(addr_pattern.size()); : } else { : continue; : } Would it be easier to read if it were written as: if (!cur_line.contains("->")) { continue; } if (HasPrefixString(cur_line.ToString(), addr_pattern)) { cur_line.remove_prefix(addr_pattern.size()); } else if (all_pattern != addr_pattern && HasPrefixString(cur_line.ToString(), all_pattern) { cur_line.remove_prefix(all_pattern.size()); } else { continue; } ? -- To view, visit http://gerrit.cloudera.org:8080/15893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib79297e0eb59cc96a91c6e301f6a70ba123f4644 Gerrit-Change-Number: 15893 Gerrit-PatchSet: 3 Gerrit-Owner: liusheng <liusheng2...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 12 May 2020 16:39:01 +0000 Gerrit-HasComments: Yes
