Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20514 )
Change subject: KUDU-3507 Fix mini ranger port detection ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/20514/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20514/2//COMMIT_MSG@10 PS2, Line 10: lsof might handle 0.0.0.0 as ipv6 > I'm not sure I understand how lsof is involved here: the address patterns p Yes. The utility function uses lsof. For example see test_utils.cc:467: string lsof; RETURN_NOT_OK(FindExecutable("lsof", {"/sbin", "/usr/sbin"}, &lsof)); I stoped the process with a breakpoint before the port detection (so it won't shut down). I tried to list the ports with "-i 4TCP". It did not show. with "-i TCP" it shows it. http://gerrit.cloudera.org:8080/#/c/20514/2//COMMIT_MSG@25 PS2, Line 25: If for some : reason an other port is opened > What might be the reason for opening some other port instead of the configu I just wanted to handle the case like the old code did. I don't know why it was done this way, but there must be a reason. http://gerrit.cloudera.org:8080/#/c/20514/2/src/kudu/ranger-kms/mini_ranger_kms.cc File src/kudu/ranger-kms/mini_ranger_kms.cc: http://gerrit.cloudera.org:8080/#/c/20514/2/src/kudu/ranger-kms/mini_ranger_kms.cc@280 PS2, Line 280: RETURN_NOT_OK(WaitForTcpBind( : process_->pid(), : &port_, : adresses, : MonoDelta::FromSeconds(0))); > If the process opens multiple (at least two) ports, how do we know that we I just wanted to make it backward compatible. There are 2 assumptions: + The API port is on 0.0.0.0. And we check it first. + Comment from test_util.cc:491: // We use the first encountered listening TCP socket, since that's most likely // to be the primary service port. When searching, we use the provided bind // address if there is any, otherwise we use '*' (same as '0.0.0.0') which // matches all addresses on the local machine. Yes it looks bad, but it worked in the last 17 months, so there must be some truth in it. http://gerrit.cloudera.org:8080/#/c/20514/2/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/20514/2/src/kudu/util/test_util.h@194 PS2, Line 194: int pid = -1 > nit: could it be std::optional<pid_t>, so no special values are introduced? Client tests use this file too. Can we update client specific files to c++17? -- To view, visit http://gerrit.cloudera.org:8080/20514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89c3409e9efd53a821a6d4244a35564e134323bb Gerrit-Change-Number: 20514 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Wed, 11 Oct 2023 07:24:09 +0000 Gerrit-HasComments: Yes
